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

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

Merged
e26chiu merged 5 commits from default-graph-prop-values into main 2022-11-16 22:00:57 -05:00
Conversation 0 Commits 5 Files Changed 2 +80 -62
e26chiu commented 2022-11-14 12:55:24 -05:00
Contributor
Copy Link

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 2022-11-14 12:55:25 -05:00
continuous-integration/drone/push Build is passing Details
4ee9ed3d8d Refactor default prop values into util file
e26chiu added 1 commit 2022-11-14 12:58:24 -05:00
e26chiu added 1 commit 2022-11-16 19:00:22 -05:00
continuous-integration/drone/push Build is passing Details
c6a71a67ad Refactor bar graph and pie chart props
snedadah approved these changes 2022-11-16 21:40:57 -05:00
snedadah left a comment
Owner
Copy Link

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
@ -30,15 +37,6 @@ export default function Demographics() {
const { width } = useWindowDimensions();
snedadah commented 2022-11-16 21:40:20 -05:00
Owner
Copy Link

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
@ -0,0 +12,4 @@
const graphWidth = (
isMobile: boolean,
width: number,
snedadah commented 2022-11-16 21:40:00 -05:00
Owner
Copy Link

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 2022-11-16 21:52:36 -05:00
continuous-integration/drone/push Build is passing Details
24c7d595a7 Rename width to pageWidth
e26chiu added 1 commit 2022-11-16 21:53:19 -05:00
e26chiu merged commit 1c0191facc into main 2022-11-16 22:00:57 -05:00
e26chiu deleted branch default-graph-prop-values 2022-11-16 22:00:58 -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#87
No description provided.
Delete Branch "default-graph-prop-values"

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?