www
/
cs-2022-class-profile
16
0
Fork 0
Code Issues 11 Pull Requests 1 Projects Releases Wiki Activity

Fix Mobile Vertical Bar Graph Labels (Closes #50) #83

Merged
e26chiu merged 10 commits from fix-mobile-bug-labels into main 2022-11-13 18:36:35 -05:00
Conversation 0 Commits 10 Files Changed 5 +48 -13
e26chiu commented 2022-11-12 11:10:51 -05:00
Contributor
Copy Link

This PR adds the following:

  • alternating label functionality to vertical bar graphs when the width of the graph reaches its minWidth
  • minWidth prop to horizontal and vertical bar graphs, by default, set to 500 (make a PR to contain these default prop values)
  • overflow scroll on mobile for horizontal and vertical bar graphs

Note: Everything that is not set to overflow scroll will be cropped (disabled horizontal scrolling in general)

In the future, some other graph components may also need the same support (minimum width + alternating labels)

image

https://fix-mobile-bug-labels-csc-class-profile-stag-snedadah.k8s.csclub.cloud/

This PR adds the following: * alternating label functionality to vertical bar graphs when the width of the graph reaches its `minWidth` * `minWidth` prop to horizontal and vertical bar graphs, by default, set to 500 (make a PR to contain these default prop values) * overflow scroll on mobile for horizontal and vertical bar graphs Note: Everything that is not set to overflow scroll will be cropped (disabled horizontal scrolling in general) In the future, some other graph components may also need the same support (minimum width + alternating labels) ![image](/attachments/bcaa29f0-25d2-447f-a18d-bb343b1d0acf) https://fix-mobile-bug-labels-csc-class-profile-stag-snedadah.k8s.csclub.cloud/
image.png
27 KiB
e26chiu added 4 commits 2022-11-12 11:10:52 -05:00
continuous-integration/drone/push Build is passing Details
6c2b1f1421 Fix min width + overflow scroll for ComponentWrapper
e26chiu added 1 commit 2022-11-12 11:22:19 -05:00
continuous-integration/drone/push Build is passing Details
4d0c0a3db1 Remove unintended change
snedadah reviewed 2022-11-12 13:55:42 -05:00
components/BarGraph.tsx
@ -44,2 +44,4 @@
/** Controls the distance between the value axis label and the value axis. */
valueAxisLabelOffset?: number;
/** Minimum width of the graph. Once the graph reaches this value,
* its content will be set to an overflow scroll */
snedadah commented 2022-11-12 13:54:47 -05:00
Owner
Copy Link

This doesnt actually set it to overflow scroll right? The "isMobile" is what sets it to overflow scroll. (Thats ok, I think its right that the overflow scroll is set by isMobile, just wanted to point out that from my understanding this comment is incorrect)

This doesnt actually set it to overflow scroll right? The "isMobile" is what sets it to overflow scroll. (Thats ok, I think its right that the overflow scroll is set by isMobile, just wanted to point out that from my understanding this comment is incorrect)
e26chiu commented 2022-11-12 15:19:55 -05:00
Author
Contributor
Copy Link

You're right, this doesn't set the overflow scroll. It's set by isMobile. minWidth only determines when the graph should stop becoming smaller. I'm going to correct this comment. I'm going to do more testing to see if it affects how graphs are viewed in different screens 🤔

You're right, this doesn't set the overflow scroll. It's set by `isMobile`. `minWidth` only determines when the graph should stop becoming smaller. I'm going to correct this comment. I'm going to do more testing to see if it affects how graphs are viewed in different screens 🤔
e26chiu marked this conversation as resolved
components/BarGraph.tsx Outdated
@ -223,3 +233,3 @@
const categoryMax = width - margin.left - margin.right;
const valueMax = height - margin.top - margin.bottom;
const valueMax = height - margin.top - margin_bottom;
snedadah commented 2022-11-12 13:40:22 -05:00
Owner
Copy Link

I think there may be a typo here, "margin_bottom"? Did you mean "margin.bottom"

I think there may be a typo here, "margin_bottom"? Did you mean "margin.bottom"
e26chiu commented 2022-11-12 14:44:18 -05:00
Author
Contributor
Copy Link

Nope, this is not a typo. Maybe I should have renamed the variables to be clearer 😅. margin_bottom is meant to be change the margin.bottom (add extra spacing) if there are alternating labels. See the definition of the margin_bottomvariabe above!

Nope, this is not a typo. Maybe I should have renamed the variables to be clearer 😅. `margin_bottom` is meant to be change the margin.bottom (add extra spacing) if there are alternating labels. See the definition of the `margin_bottom`variabe above!
e26chiu marked this conversation as resolved
components/BarGraph.tsx Outdated
@ -311,1 +321,3 @@
tickLabelProps={() => {
tickLabelProps={(value, index) => {
const defaultDy = `-0.25rem`;
const alternatingDy = index % 2 == 0 ? defaultDy : `25px`;
snedadah commented 2022-11-12 13:43:35 -05:00
Owner
Copy Link

On my phone, the "Slabo 27px" which is two lines overlaps "Raleway". So I think we should maybe increase the spacing a bit more? Furtheremore what would happen if there was a 3 line text in the top row? (Maybe we should provide the spacing as a prop so if needed we can increase the spacing to fit the data if there is a 3 line text)

Also is there a reason for using "rem" in one value, but "px" in the other? We should probably just stick to one.

On my phone, the "Slabo 27px" which is two lines overlaps "Raleway". So I think we should maybe increase the spacing a bit more? Furtheremore what would happen if there was a 3 line text in the top row? (Maybe we should provide the spacing as a prop so if needed we can increase the spacing to fit the data if there is a 3 line text) Also is there a reason for using "rem" in one value, but "px" in the other? We should probably just stick to one.
👍 1
e26chiu commented 2022-11-13 17:18:00 -05:00
Author
Contributor
Copy Link

@snedadah It seems that "rem" are not supported units in SVGs. If you inspect the labels, they are all nested inside of SVGs. dy sets the y attribute of the SVGs. So the -0.25rem surprisingly does nothing 🤔. I'll opt for px for now.
Otherwise, thanks for noticing! Made these values props to adjust spacing!

@snedadah It seems that ["rem" are not supported units in SVGs](https://bugzilla.mozilla.org/show_bug.cgi?id=1231147). If you inspect the labels, they are all nested inside of SVGs. `dy` sets the `y` attribute of the SVGs. So the `-0.25rem` surprisingly does nothing 🤔. I'll opt for `px` for now. Otherwise, thanks for noticing! Made these values props to adjust spacing!
components/ComponentWrapper.tsx Outdated
@ -38,3 +43,3 @@
{bodyText ? <p>{bodyText}</p> : null}
</div>
<div className={styles.internalWrapper}>{children}</div>
<div className={styles.internalWrapper} style={overflowScroll}>
snedadah commented 2022-11-12 13:48:46 -05:00
Owner
Copy Link

Is there a reason for not using the already built in media queries here?

I reccomend deleting all these changes here, and instead adding the changes to the css if possilbe. It will look something like this:

.internalWrapper {
  padding: calc(20rem / 16);
  overflow: "visible";
}
@media screen and (max-width: 900px) {
  .sideWrapperCommon {
    margin: auto;
    flex-direction: column;
    text-align: center;
    padding: 0;
    border-radius: 0;
    width: 100%;
  }

  .wrapperCenter {
    padding: 0;
  }
  .internalWrapper {
  	overflow: "scroll";
  }
}

Note that you will have to put the default ".interalWrapper" above the @media, since you want the @media .internalWrapper to override the top one.

Is there a reason for not using the already built in media queries here? I reccomend deleting all these changes here, and instead adding the changes to the css if possilbe. It will look something like this: ``` .internalWrapper { padding: calc(20rem / 16); overflow: "visible"; } @media screen and (max-width: 900px) { .sideWrapperCommon { margin: auto; flex-direction: column; text-align: center; padding: 0; border-radius: 0; width: 100%; } .wrapperCenter { padding: 0; } .internalWrapper { overflow: "scroll"; } } ``` Note that you will have to put the default ".interalWrapper" above the @media, since you want the @media .internalWrapper to override the top one.
👍 1
e26chiu marked this conversation as resolved
pages/_app.css Outdated
@ -68,6 +68,7 @@ body {
color: var(--primary-text);
font-family: "Inconsolata", monospace;
margin: 0;
overflow-x: hidden;
snedadah commented 2022-11-12 13:51:13 -05:00
Owner
Copy Link

The section header at the top "Demographics" still caused my phone to zoom out (basically overflow-x) Just a note that we need to put a media query to fix that.

The section header at the top "Demographics" still caused my phone to zoom out (basically overflow-x) Just a note that we need to put a media query to fix that.
👍 1
e26chiu added 4 commits 2022-11-13 17:17:44 -05:00
snedadah added 1 commit 2022-11-13 18:14:33 -05:00
continuous-integration/drone/push Build is passing Details
2051679458 Updated style name and demographics page spacing
snedadah approved these changes 2022-11-13 18:27:04 -05:00
snedadah left a comment
Owner
Copy Link

Nice work! I just made a tiny change in 2051679458 , removing the "nth child" since that is not maintainble, everything else looks good!

Nice work! I just made a tiny change in https://git.csclub.uwaterloo.ca/www/cs-2022-class-profile/commit/2051679458156180a3c003b665ceec25681f2559 , removing the "nth child" since that is not maintainble, everything else looks good!
👍 1
e26chiu merged commit aa25b8451b into main 2022-11-13 18:36:35 -05:00
e26chiu deleted branch fix-mobile-bug-labels 2022-11-13 18:36:36 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
snedadah
Labels
Clear labels   
Bug

   
Component

   
Config

   
Good First Issue

   
Low-Priority

   
Page
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#83
No description provided.
Delete Branch "fix-mobile-bug-labels"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?