Bar Graph Component #16

Merged
a258wang merged 16 commits from a258wang-bar-graph into main 2 months ago
Owner

Adds a <BarGraphHorizontal /> component and a <BarGraphVertical /> component.

Closes #1.

Possible changes for the future:

  • Refactor to make the horizontal and vertical bar graphs into one component
  • Add (optional) graph title

https://a258wang-bar-graph-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground/

Adds a `<BarGraphHorizontal />` component and a `<BarGraphVertical />` component. Closes #1. Possible changes for the future: - Refactor to make the horizontal and vertical bar graphs into one component - Add (optional) graph title https://a258wang-bar-graph-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground/
a258wang added 8 commits 4 months ago
a258wang force-pushed a258wang-bar-graph from 5709c76a81 to da3c9f92bb 3 months ago
a258wang force-pushed a258wang-bar-graph from da3c9f92bb to 293183064a 3 months ago
a258wang added 1 commit 3 months ago
77d0a8a93d Add optional axis labels
a258wang changed title from WIP: Bar Graph Component to Bar Graph Component 3 months ago
a258wang requested review from j285he 3 months ago
a258wang requested review from snedadah 3 months ago
a258wang requested review from b72zhou 3 months ago
a258wang requested review from e26chiu 3 months ago
snedadah approved these changes 2 months ago
snedadah left a comment
Collaborator

Looks good to me! And yeah we definitely should add the title (though maybe we should instead have some sort of wrapper title component for all the graphs)

Looks good to me! And yeah we definitely should add the title (though maybe we should instead have some sort of wrapper title component for all the graphs)
.barText {
visibility: hidden;
font-family: "Inconsolata";
Collaborator

Do we need to specify the font family on all of these even though the font is set globally for the page?
Also small nit: If we do need to set the font, should we add the monospace fallback here?

Do we need to specify the font family on all of these even though the font is set globally for the page? Also small nit: If we do need to set the font, should we add the monospace fallback here?
Poster
Owner

I just tried it, and it looks like the visx Text component automatically uses Arial if you don't specify a font, so we either have to pass the font family as a prop to the Text component, or we have to specify the font on each class as done here.

Good catch on adding the monospace fallback though.

I just tried it, and it looks like the visx Text component automatically uses Arial if you don't specify a font, so we either have to pass the font family as a prop to the Text component, or we have to specify the font on each class as done here. Good catch on adding the monospace fallback though.
a258wang added 1 commit 2 months ago
21188553a8 Add monospace fallback
Poster
Owner

I'm going to merge this without the graph title for now, and we can always come back later to add it if we want - in the meantime, throwing a heading above the graph would work in a pinch lol

I'm going to merge this without the graph title for now, and we can always come back later to add it if we want - in the meantime, throwing a heading above the graph would work in a pinch lol
a258wang merged commit 6735c52914 into main 2 months ago
a258wang deleted branch a258wang-bar-graph 2 months ago
a258wang referenced this issue from a commit 2 months ago

Reviewers

j285he was requested for review 3 months ago
b72zhou was requested for review 3 months ago
e26chiu was requested for review 3 months ago
snedadah approved these changes 2 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 6735c52914.
Sign in to join this conversation.
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#16
Loading…
There is no content yet.