Line Graph Component using curve #47

Merged
b72zhou merged 25 commits from b72zhou-line-graph-v2 into main 2 months ago
Collaborator
https://b72zhou-line-graph-v2-csc-class-profile-staging-snedadah.k8s.csclub.cloud/
b72zhou added 1 commit 3 months ago
ac4a117ab7 add axis
b72zhou added 1 commit 3 months ago
a21d4cc453 add grid & adjust margin & add hover effect
b72zhou changed title from WIP: Line Graph Component using curve to Line Graph Component using curve 3 months ago
Poster
Collaborator
  • Add Tooltip
- [x] Add Tooltip
b72zhou added 1 commit 3 months ago
934ab39dba add tooltip
b72zhou added 2 commits 3 months ago
421a474aa1 fix conflict
b72zhou added 1 commit 3 months ago
99658f871c fix package
b72zhou added 2 commits 3 months ago
766dfd9791 fix conflict
b72zhou added 1 commit 3 months ago
2d8121ab5b merge playground
b72zhou added 1 commit 3 months ago
add5bf88e9 fix lint
b72zhou requested review from j285he 3 months ago
b72zhou requested review from a258wang 3 months ago
b72zhou requested review from snedadah 3 months ago
b72zhou requested review from e26chiu 3 months ago
a258wang reviewed 3 months ago
a258wang left a comment
Owner

This actually looks so good, thank you Beihao for working on it! Just left some small questions and nitpicks 🙂

This actually looks so good, thank you Beihao for working on it! Just left some small questions and nitpicks 🙂
const yScale = scaleLinear<number>({
range: [0, yMax],
nice: true,
domain: [100, 0],
Owner

This should probably be [<maxYValue>, 0] in case we go above 100 (eg. if we aren't doing percentages)

This should probably be `[<maxYValue>, 0]` in case we go above 100 (eg. if we aren't doing percentages)
b72zhou marked this conversation as resolved
<GridRows
scale={yScale}
width={xMax}
left={margin.left * 2}
Owner

Since everything is already inside the Group with top={margin.top} left={margin.left}, I'm not sure if you still need to set all the margins on the components since they should be positioned relative to the Group.

In particular, I noticed that the horizontal grid lines stick out a little bit on each side, when they should ideally not stick out past the vertical lines on both ends. Hopefully this is possible!

Since everything is already inside the `Group` with `top={margin.top} left={margin.left}`, I'm not sure if you still need to set all the margins on the components since they should be positioned relative to the `Group`. In particular, I noticed that the horizontal grid lines stick out a little bit on each side, when they should ideally not stick out past the vertical lines on both ends. Hopefully this is possible!
Poster
Collaborator

oh the reason why I still set margins on components is to stick out past the vertical lines on both ends lol. So I think we may still need the margin and I adjusted the margin a bit more to make it look better

oh the reason why I still set margins on components is to stick out past the vertical lines on both ends lol. So I think we may still need the margin and I adjusted the margin a bit more to make it look better
b72zhou marked this conversation as resolved
/>
<Group left={margin.left + xMax / (data.xValues.length * 2)}>
{actualData.map((lineData, i) => {
const even = i % 2 === 0;
Owner

NIT: some people prefer to name booleans isEven etc.

NIT: some people prefer to name booleans `isEven` etc.
b72zhou marked this conversation as resolved
data={lineData}
className={styles.line}
x={(d) => xScale(getX(d)) ?? 0}
y={(d) => yScale(getY(d)) ?? 0}
Owner

These can probably just be getX(d)! etc. instead of ??

These can probably just be `getX(d)!` etc. instead of ??
Poster
Collaborator

I tried but it says non-null assertion is forbidden. Do we have any other ideas?

I tried but it says non-null assertion is forbidden. Do we have any other ideas?
Owner

Use // eslint-disable-next-line @typescript-eslint/no-non-null-assertion

We should probably disable this lint rule globally, but the comment should be good enough for now.

Use `// eslint-disable-next-line @typescript-eslint/no-non-null-assertion` We should probably disable this lint rule globally, but the comment should be good enough for now.
b72zhou marked this conversation as resolved
x={(d) => xScale(getX(d)) ?? 0}
y={(d) => yScale(getY(d)) ?? 0}
stroke={even ? Color.primaryAccent : Color.secondaryAccent}
strokeWidth={3}
Owner

How does it look with strokeWidth={4}? I just think it's a little funny that the gridlines are thicker than the actual chart lines. 😂

How does it look with `strokeWidth={4}`? I just think it's a little funny that the gridlines are thicker than the actual chart lines. 😂
b72zhou marked this conversation as resolved
</svg>
{tooltipOpen && (
<TooltipInPortal
Owner

I'm curious, what's the difference between Tooltip vs. TooltipInPortal? Just asking because I saw everyone else using Tooltip. XD

I'm curious, what's the difference between `Tooltip` vs. `TooltipInPortal`? Just asking because I saw everyone else using Tooltip. XD
Poster
Collaborator

The documentation for TooltipInPortal says:

For example, if your chart is rendered inside a stacking context with a lower z-index than a surrounding container, it may get clipped by that container even if you specify a higher z-index. This is solvable with a Portal because the separate container will not be subject to the stacking context of your chart.

Since I met problems with hover effect before, it seems this might help if I met similar problems again. But actually everything goes well this time and the feature for portal is not used. XD

The documentation for `TooltipInPortal` says: > For example, if your chart is rendered inside a stacking context with a lower z-index than a surrounding container, it may get clipped by that container even if you specify a higher z-index. This is solvable with a Portal because the separate container will not be subject to the stacking context of your chart. Since I met problems with hover effect before, it seems this might help if I met similar problems again. But actually everything goes well this time and the feature for portal is not used. XD
b72zhou marked this conversation as resolved
{tooltipOpen && (
<TooltipInPortal
// set this to random so it correctly updates with parent bounds
Owner

this comment seems outdated 🤔

this comment seems outdated 🤔
b72zhou marked this conversation as resolved
data/mocks.ts Outdated
export const mockLineData = {
xValues: ["1A", "1B", "2A", "2B", "3A", "3B"],
lines: [
Owner

Maybe we should throw an error in the LineGraph component if the number of xValues and number of yValues don't match?

Maybe we should throw an error in the LineGraph component if the number of xValues and number of yValues don't match?
b72zhou marked this conversation as resolved
left: 40,
right: 20,
}}
></LineGraph>
Owner

NIT: <LineGraph />

NIT: `<LineGraph />`
b72zhou marked this conversation as resolved
b72zhou added 2 commits 3 months ago
ebd713afd2 resolve conf
b72zhou added 1 commit 3 months ago
85fbfb4c7e refactor: review
b72zhou added 1 commit 3 months ago
e02df94e28 fix lint
b72zhou added 1 commit 3 months ago
a7bdc9a069 Merge branch 'main' into b72zhou-line-graph-v2
b72zhou added 2 commits 3 months ago
b72zhou added 1 commit 3 months ago
1642f5651c fix: all review comments
b72zhou added 1 commit 3 months ago
6df4dfdb05 feat: add legend
b72zhou added 1 commit 3 months ago
bfcb7b465a fix lint
b72zhou added 3 commits 3 months ago
Poster
Collaborator

@a258wang I feel that the staging doesn't work... Could you please npm run dev locally and see if we can merge this component? Thanks!

@a258wang I feel that the staging doesn't work... Could you please `npm run dev` locally and see if we can merge this component? Thanks!
a258wang removed review request for j285he 2 months ago
a258wang removed review request for e26chiu 2 months ago
a258wang approved these changes 2 months ago
b72zhou added 2 commits 2 months ago
dc59d058df merge main
b72zhou scheduled this pull request to auto merge when all checks succeed 2 months ago
b72zhou merged commit d8867d3c86 into main 2 months ago
b72zhou deleted branch b72zhou-line-graph-v2 2 months ago

Reviewers

snedadah was requested for review 3 months ago
a258wang approved these changes 2 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as d8867d3c86.
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#47
Loading…
There is no content yet.