Refactor default prop values into util file (Closes #82) #87

Merged
e26chiu merged 5 commits from default-graph-prop-values into main 3 months ago
Collaborator

This PR creates a new util file DefaultProp that provides default prop values for graph/chart components. More values could be added in the future as the pages are developed.

staging: (shahan: sorry, due to a unlucky naming this branch name doesnt work with staging )

This PR creates a new util file `DefaultProp` that provides default prop values for graph/chart components. More values could be added in the future as the pages are developed. staging: (shahan: sorry, due to a unlucky naming this branch name doesnt work with staging )
e26chiu added 1 commit 3 months ago
4ee9ed3d8d Refactor default prop values into util file
e26chiu added 1 commit 3 months ago
e26chiu added 1 commit 3 months ago
c6a71a67ad Refactor bar graph and pie chart props
snedadah approved these changes 3 months ago
snedadah left a comment
Owner

LGTM, pending renaming width to "pageWidth". Let's add the dynamic components in a seperate pr.

LGTM, pending renaming width to "pageWidth". Let's add the dynamic components in a seperate pr.
const { width } = useWindowDimensions();
Owner

as mentioned before, this should be renamed to "pageWidth"

as mentioned before, this should be renamed to "pageWidth"
e26chiu marked this conversation as resolved
const graphWidth = (
isMobile: boolean,
width: number,
Owner

I think all the cases of width that refer to the page width, such as this one, ones below and above, should be renamed to pageWidth. On first glance, its pretty confusing to read this code without realizing that "width" refers to the page width (and not the desired witdh of the graph)
Especially for the barGraphProps function.

I think all the cases of width that refer to the page width, such as this one, ones below and above, should be renamed to pageWidth. On first glance, its pretty confusing to read this code without realizing that "width" refers to the page width (and not the desired witdh of the graph) Especially for the barGraphProps function.
e26chiu marked this conversation as resolved
e26chiu added 1 commit 3 months ago
24c7d595a7 Rename width to pageWidth
e26chiu added 1 commit 3 months ago
e26chiu merged commit 1c0191facc into main 3 months ago
e26chiu deleted branch default-graph-prop-values 3 months ago

Reviewers

snedadah approved these changes 3 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 1c0191facc.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#87
Loading…
There is no content yet.