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

Line Graph Component using curve #47

Merged
b72zhou merged 25 commits from b72zhou-line-graph-v2 into main 2022-10-02 15:01:18 -04:00
Conversation 2 Commits 25 Files Changed 7 +2634 -35
b72zhou commented 2022-08-31 23:45:03 -04:00
Contributor
Copy Link

https://b72zhou-line-graph-v2-csc-class-profile-staging-snedadah.k8s.csclub.cloud/

https://b72zhou-line-graph-v2-csc-class-profile-staging-snedadah.k8s.csclub.cloud/
b72zhou added 1 commit 2022-08-31 23:45:05 -04:00
continuous-integration/drone/push Build is failing Details
ac4a117ab7 add axis
b72zhou added 1 commit 2022-09-01 23:46:23 -04:00
continuous-integration/drone/push Build is passing Details
a21d4cc453 add grid & adjust margin & add hover effect
b72zhou changed title from WIP: Line Graph Component using curve to Line Graph Component using curve 2022-09-01 23:55:18 -04:00
b72zhou commented 2022-09-01 23:56:11 -04:00
Author
Contributor
Copy Link
  • Add Tooltip
- [x] Add Tooltip
b72zhou added 1 commit 2022-09-03 01:13:21 -04:00
continuous-integration/drone/push Build is failing Details
934ab39dba add tooltip
b72zhou added 2 commits 2022-09-03 01:20:09 -04:00
continuous-integration/drone/push Build is failing Details
421a474aa1 fix conflict
b72zhou added 1 commit 2022-09-03 01:24:13 -04:00
continuous-integration/drone/push Build is failing Details
99658f871c fix package
b72zhou added 2 commits 2022-09-03 01:37:51 -04:00
continuous-integration/drone/push Build is failing Details
766dfd9791 fix conflict
b72zhou added 1 commit 2022-09-03 01:44:19 -04:00
continuous-integration/drone/push Build is failing Details
2d8121ab5b merge playground
b72zhou added 1 commit 2022-09-03 01:46:19 -04:00
continuous-integration/drone/push Build is passing Details
add5bf88e9 fix lint
b72zhou requested review from j285he 2022-09-03 01:47:11 -04:00
b72zhou requested review from a258wang 2022-09-03 01:47:11 -04:00
b72zhou requested review from snedadah 2022-09-03 01:47:12 -04:00
b72zhou requested review from e26chiu 2022-09-03 01:47:12 -04:00
a258wang reviewed 2022-09-06 02:19:46 -04:00
a258wang left a comment
Owner
Copy Link

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 🙂
components/LineGraph.tsx Outdated
@ -0,0 +122,4 @@
const yScale = scaleLinear<number>({
range: [0, yMax],
nice: true,
domain: [100, 0],
a258wang commented 2022-09-06 02:17:35 -04:00
Owner
Copy Link

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
components/LineGraph.tsx Outdated
@ -0,0 +142,4 @@
<GridRows
scale={yScale}
width={xMax}
left={margin.left * 2}
a258wang commented 2022-09-06 02:15:50 -04:00
Owner
Copy Link

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!
b72zhou commented 2022-09-11 21:47:41 -04:00
Author
Contributor
Copy Link

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
components/LineGraph.tsx
@ -0,0 +183,4 @@
/>
<Group left={margin.left + xMax / (data.xValues.length * 2)}>
{actualData.map((lineData, i) => {
const even = i % 2 === 0;
a258wang commented 2022-09-06 02:08:54 -04:00
Owner
Copy Link

NIT: some people prefer to name booleans isEven etc.

NIT: some people prefer to name booleans `isEven` etc.
b72zhou marked this conversation as resolved
components/LineGraph.tsx
@ -0,0 +205,4 @@
data={lineData}
className={styles.line}
x={(d) => xScale(getX(d)) ?? 0}
y={(d) => yScale(getY(d)) ?? 0}
a258wang commented 2022-09-06 02:08:07 -04:00
Owner
Copy Link

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

These can probably just be `getX(d)!` etc. instead of ??
b72zhou commented 2022-09-06 14:06:33 -04:00
Author
Contributor
Copy Link

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?
a258wang commented 2022-09-07 00:19:56 -04:00
Owner
Copy Link

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.
👍 1
b72zhou marked this conversation as resolved
components/LineGraph.tsx Outdated
@ -0,0 +207,4 @@
x={(d) => xScale(getX(d)) ?? 0}
y={(d) => yScale(getY(d)) ?? 0}
stroke={even ? Color.primaryAccent : Color.secondaryAccent}
strokeWidth={3}
a258wang commented 2022-09-06 02:09:58 -04:00
Owner
Copy Link

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
components/LineGraph.tsx Outdated
@ -0,0 +218,4 @@
</svg>
{tooltipOpen && (
<TooltipInPortal
a258wang commented 2022-09-06 02:05:38 -04:00
Owner
Copy Link

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
b72zhou commented 2022-09-11 21:54:17 -04:00
Author
Contributor
Copy Link

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
👍 1
b72zhou marked this conversation as resolved
components/LineGraph.tsx Outdated
@ -0,0 +219,4 @@
{tooltipOpen && (
<TooltipInPortal
// set this to random so it correctly updates with parent bounds
a258wang commented 2022-09-06 02:04:14 -04:00
Owner
Copy Link

this comment seems outdated 🤔

this comment seems outdated 🤔
b72zhou marked this conversation as resolved
data/mocks.ts Outdated
@ -67,1 +67,4 @@
export const mockLineData = {
xValues: ["1A", "1B", "2A", "2B", "3A", "3B"],
lines: [
a258wang commented 2022-09-06 02:19:07 -04:00
Owner
Copy Link

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
pages/playground.tsx Outdated
@ -70,0 +82,4 @@
left: 40,
right: 20,
}}
></LineGraph>
a258wang commented 2022-09-06 02:03:08 -04:00
Owner
Copy Link

NIT: <LineGraph />

NIT: `<LineGraph />`
b72zhou marked this conversation as resolved
b72zhou added 2 commits 2022-09-06 13:55:51 -04:00
continuous-integration/drone/push Build is failing Details
ebd713afd2 resolve conf
b72zhou added 1 commit 2022-09-07 21:27:26 -04:00
continuous-integration/drone/push Build is failing Details
85fbfb4c7e refactor: review
b72zhou added 1 commit 2022-09-07 21:46:20 -04:00
continuous-integration/drone/push Build is passing Details
e02df94e28 fix lint
b72zhou added 1 commit 2022-09-11 21:15:17 -04:00
continuous-integration/drone/push Build is passing Details
a7bdc9a069 Merge branch 'main' into b72zhou-line-graph-v2
b72zhou added 2 commits 2022-09-11 21:28:03 -04:00
b72zhou added 1 commit 2022-09-11 21:58:47 -04:00
continuous-integration/drone/push Build is passing Details
1642f5651c fix: all review comments
b72zhou added 1 commit 2022-09-11 23:02:13 -04:00
continuous-integration/drone/push Build is passing Details
6df4dfdb05 feat: add legend
b72zhou added 1 commit 2022-09-14 15:49:14 -04:00
continuous-integration/drone/push Build is failing Details
bfcb7b465a fix lint
b72zhou added 3 commits 2022-09-14 16:06:40 -04:00
continuous-integration/drone/push Build is passing Details
b9552facb0 merge main
b72zhou commented 2022-09-27 19:49:38 -04:00
Author
Contributor
Copy Link

@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 2022-09-29 01:22:39 -04:00
a258wang removed review request for e26chiu 2022-09-29 01:22:41 -04:00
a258wang approved these changes 2022-09-30 01:42:54 -04:00
b72zhou added 2 commits 2022-10-02 14:57:12 -04:00
continuous-integration/drone/push Build is passing Details
dc59d058df merge main
b72zhou scheduled this pull request to auto merge when all checks succeed 2022-10-02 14:57:30 -04:00
b72zhou merged commit d8867d3c86 into main 2022-10-02 15:01:18 -04:00
b72zhou deleted branch b72zhou-line-graph-v2 2022-10-02 15:03:18 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
snedadah
a258wang
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#47
No description provided.
Delete Branch "b72zhou-line-graph-v2"

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?