www
/
cs-2022-class-profile
16
0
Fork 0
Code Issues 23 Pull Requests 4 Projects Releases Wiki Activity
Labels Milestones

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
Conversation 0 Commits 5 Files Changed 2
e26chiu commented 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
continuous-integration/drone/push Build is passing
Details
4ee9ed3d8d Refactor default prop values into util file
e26chiu added 1 commit 3 months ago
e26chiu added 1 commit 3 months ago
continuous-integration/drone/push Build is passing
Details
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.
👍 1
pages/demographics.tsx Outdated
const { width } = useWindowDimensions();
snedadah commented 3 months ago
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
utils/defaultProps.ts Outdated
const graphWidth = (
isMobile: boolean,
width: number,
snedadah commented 3 months ago
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.
👍 1
e26chiu marked this conversation as resolved
e26chiu added 1 commit 3 months ago
continuous-integration/drone/push Build is passing
Details
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
Details
The pull request has been merged as 1c0191facc.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
snedadah
Labels
Apply labels
Clear labels
Bug Component Config Good First Issue Low-Priority Page
No Label Bug Component Config Good First Issue Low-Priority Page
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#87
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'default-graph-prop-values'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes