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

Pie chart component #19

Merged
j285he merged 23 commits from j285he-pie-chart into main 5 months ago
Conversation 3 Commits 23 Files Changed 5
j285he commented 8 months ago
Collaborator

Closes #5.

To fix:

  • Inner pie slice text not perfectly centered

  • Create optional props for padRadius and innerRadius

  • Change colors to global colors and merge from main to get fonts

Also, outer labels can get cut off if they are long enough, but the labelWidth is provided as a prop for the user to adjust.

Staging url: https://j285he-pie-chart-csc-class-profile-staging-snedadah.k8s.csclub.cloud

Closes #5. To fix: - [x] Inner pie slice text not perfectly centered - [x] Create optional props for `padRadius` and `innerRadius` - [x] Change colors to global colors and merge from main to get fonts Also, outer labels can get cut off if they are long enough, but the `labelWidth` is provided as a prop for the user to adjust. Staging url: https://j285he-pie-chart-csc-class-profile-staging-snedadah.k8s.csclub.cloud
j285he added 7 commits 8 months ago
j285he added 1 commit 8 months ago
j285he commented 8 months ago
Poster
Collaborator

This is what the pie looks like hovered on a small slice.

This is what the pie looks like hovered on a small slice.
Screenshot 2022-06-18 230454.png
19 KiB
j285he closed this pull request 8 months ago
j285he reopened this pull request 8 months ago
j285he added 4 commits 7 months ago
continuous-integration/drone/push Build is passing
Details
4cc58ca27a Spacing
j285he changed title from WIP: Pie chart component to Pie chart component 7 months ago
j285he requested review from n3parikh 7 months ago
j285he requested review from a258wang 7 months ago
j285he requested review from snedadah 7 months ago
j285he requested review from b72zhou 7 months ago
j285he requested review from e26chiu 7 months ago
j285he added 2 commits 7 months ago
continuous-integration/drone/push Build is passing
Details
4fdaefef82 Change margins_
a258wang commented 7 months ago
Owner

I noticed in staging that the "glow" effect only appears on the inside of the pie slice - the outer edge remains smooth. Ideally, we'd like the "glow" to be even around the entire slice when hovering.

image

If you could investigate and come up with a fix, that'd be great! Let me know if you want any help though 🙂

I noticed in staging that the "glow" effect only appears on the inside of the pie slice - the outer edge remains smooth. Ideally, we'd like the "glow" to be even around the entire slice when hovering. ![image](/attachments/3b925bf2-540f-44eb-9889-77ed9515154f) If you could investigate and come up with a fix, that'd be great! Let me know if you want any help though 🙂
image.png
18 KiB
j285he added 3 commits 7 months ago
continuous-integration/drone/push Build is passing
Details
6735c52914 Bar Graph Component (#16)
continuous-integration/drone/push Build is passing
Details
fc3512f72f Fix shadow not working
a258wang requested changes 6 months ago
a258wang left a comment
Owner

Oh no, I left some comments a few weeks ago but it looks like they got stuck in a "Pending" state... 😢 Hopefully they show up now! Apologies for the extra-delayed review 😞

Oh no, I left some comments a few weeks ago but it looks like they got stuck in a "Pending" state... 😢 Hopefully they show up now! Apologies for the extra-delayed review 😞
components/PieChart.module.css
.labelPath {
fill: var(--primary-background);
fill-opacity: 0;
a258wang commented 6 months ago
Owner

Just curious, does fill matter if fill-opacity is set to 0?

Just curious, does `fill` matter if `fill-opacity` is set to 0?
j285he marked this conversation as resolved
components/PieChart.module.css
.labelText {
fill: var(--label);
font-size: 40px;
font-weight: 800;
a258wang commented 6 months ago
Owner

NITs:

  • calc(40 rem / 16)
  • should this be font-weight 700 (bold)?
NITs: - `calc(40 rem / 16)` - should this be font-weight 700 (bold)?
a258wang commented 6 months ago
Owner

Also should/can we make the font size a prop that can be tweaked programmatically? Visx has a Text component that takes textSize as a prop, among other style attributes.

Also should/can we make the font size a prop that can be tweaked programmatically? Visx has a Text component that takes textSize as a prop, among other style attributes.
j285he marked this conversation as resolved
components/PieChart.module.css
.group:hover > .piePath {
fill: var(--primary-accent);
filter: drop-shadow(0px 0px 6px var(--primary-accent));
a258wang commented 6 months ago
Owner

NIT: 0 0 calc(6rem / 16) var(--primary-accent)

NIT: `0 0 calc(6rem / 16) var(--primary-accent)`
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
width: number;
labelWidth: number;
padRadius?: number;
innerRadius?: number;
a258wang commented 7 months ago
Owner

Suggestion: Add JSDoc comments above the less obvious props to describe what they do, like this:

/** description for the padRadius prop */
padRadius?: number

This will be helpful for people using your component in the future, since VSCode can detect this information and display it on hover 🙂

Suggestion: Add JSDoc comments above the less obvious props to describe what they do, like this: ```typescript /** description for the padRadius prop */ padRadius?: number ``` This will be helpful for people using your component in the future, since VSCode can detect this information and display it on hover 🙂
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
}: PieChartProps) {
const pieWidth = width * 0.5 - labelWidth;
return (
<svg className={props.className} width={width} height={width}>
a258wang commented 7 months ago
Owner

NIT: include className when destructuring the other props above

NIT: include `className` when destructuring the other props above
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
<svg className={props.className} width={width} height={width}>
<Group top={width * 0.5} left={width * 0.5}>
<Pie
data={props.data}
a258wang commented 7 months ago
Owner

NIT: same thing, destructuring

In general, my opionion is to either destructure all the props for the component, or none of them.

NIT: same thing, destructuring In general, my opionion is to either destructure all the props for the component, or none of them.
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
isLabel: boolean;
};
export function PieSlices({ isLabel, ...props }: PieSlicesProps<PieChartData>) {
a258wang commented 6 months ago
Owner

NIT: maybe name this component PieSlice since it's just one slice of the pie?

NIT: maybe name this component `PieSlice` since it's just one slice of the pie?
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
const pathArc = props.path(arc) as string;
return (
<g className={styles.group} key={`arc-${arc.data.category}`}>
a258wang commented 6 months ago
Owner

Visx has a Group component which eventually renders a g element, is there a reason why we're writing raw SVG stuff here instead of using the Group?

(My guess is that visx components need to be inside an svg, though apparently it's fine to nest svgs so we could just throw one of those here?)

(My second guess is because there's no visx equivalent of the path element used below (?), so we're just being consistent by using only SVG elements here?`

Visx has a `Group` component which eventually renders a `g` element, is there a reason why we're writing raw SVG stuff here instead of using the `Group`? (My guess is that visx components need to be inside an `svg`, though apparently it's fine to nest `svg`s so we could just throw one of those here?) (My second guess is because there's no visx equivalent of the `path` element used below (?), so we're just being consistent by using only SVG elements here?`
j285he commented 6 months ago
Poster
Collaborator

Third option: I forgot about Group 😅

Third option: I forgot about Group 😅
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
d={pathArc}
/>
<text
className={isLabel ? styles.labelText : styles.pieText}
a258wang commented 6 months ago
Owner

I noticed that we have a lot of styles etc. that are conditional on the isLabel prop. Have we considered passing in modifierClassName as a prop, and use CSS selectors to apply the styles without branching on a boolean all the time? eg.

<PieSlices modifierClassName={styles.label} />
...
// in the PieSlices component
<text className={`${modifierClassName} styles.text`} />

/* in css */
.label.text {
  ...
}

Numeric offsets like the +10 in centroidY + 10 could also be passed in as a prop. Even the displayed text value like arc.data.category could be obtained using a function that is passed in as a prop:

<PieSlices getDisplayValueFromDatum={(datum: PieChartData) => `${datum.value}%`} />
...
// in the PieSlices component
<text>
  {getDisplayValueFromDatum(arc.data)}
</text>

My main qualm about making everything conditional on isLabel is that if we ever wanted to add a third or fourth type of pie slice with subtly different styles/values/etc., then the approach of branching on a boolean doesn't scale very well. That being said, if you think this is a tad overkill, feel free to ignore my suggestions. 🙂

I noticed that we have a lot of styles etc. that are conditional on the `isLabel` prop. Have we considered passing in `modifierClassName` as a prop, and use CSS selectors to apply the styles without branching on a boolean all the time? eg. ```react <PieSlices modifierClassName={styles.label} /> ... // in the PieSlices component <text className={`${modifierClassName} styles.text`} /> ``` ```css /* in css */ .label.text { ... } ``` Numeric offsets like the `+10` in `centroidY + 10` could also be passed in as a prop. Even the displayed text value like `arc.data.category` could be obtained using a function that is passed in as a prop: ```react <PieSlices getDisplayValueFromDatum={(datum: PieChartData) => `${datum.value}%`} /> ... // in the PieSlices component <text> {getDisplayValueFromDatum(arc.data)} </text> ``` My main qualm about making everything conditional on `isLabel` is that if we ever wanted to add a third or fourth type of pie slice with subtly different styles/values/etc., then the approach of branching on a boolean doesn't scale very well. That being said, if you think this is a tad overkill, feel free to ignore my suggestions. 🙂
j285he commented 6 months ago
Poster
Collaborator

I fixed the centroidY issue.

For isLabel I'm running into a few issues.

We will still have conditionals in places like

...
              x={isLabel ? centroidX : centroidX + pieTextXOffset}
              y={isLabel ? centroidY : centroidY + pieTextYOffset}
              textAnchor="middle"
              fontSize={isLabel ? labelTextSize : pieTextSize}
            >
              {isLabel ? `${arc.data.category}` : `${arc.data.value}%`}
            </Text>

Is this ok?

Also, this approach means we want a "default behavior" (.path and .text) and the ability to add new classes (.label), right?

However, the text inside the pie actually needs to be hidden, which means either:

  • We make the .path and .text behavior for the inner pie to be default. This will be unintuitive as text will be hidden by default.
  • We make the .path and .text behavior for the label to be default. However, label path is hidden by default.
  • We can make pie's .path behavior default and label's .text behavior default, and create two new props: pathModifierClassName and textModifierClassName and assign pathModifierClassName="label" and textModifierClassName="pie" as default. I'm going with this one for now.
  • Something I'm missing?

I think most of these issues could actually be simpler if we just separate PieSlice into PieSlice and PieLabel. Thoughts?

I fixed the `centroidY` issue. For `isLabel` I'm running into a few issues. We will still have conditionals in places like ``` ... x={isLabel ? centroidX : centroidX + pieTextXOffset} y={isLabel ? centroidY : centroidY + pieTextYOffset} textAnchor="middle" fontSize={isLabel ? labelTextSize : pieTextSize} > {isLabel ? `${arc.data.category}` : `${arc.data.value}%`} </Text> ``` Is this ok? Also, this approach means we want a "default behavior" (`.path` and `.text`) and the ability to add new classes (`.label`), right? However, the text inside the pie actually needs to be hidden, which means either: * We make the `.path` and `.text` behavior for the inner pie to be default. This will be unintuitive as text will be hidden by default. * We make the `.path` and `.text` behavior for the label to be default. However, label path is hidden by default. * We can make pie's `.path` behavior default and label's `.text` behavior default, and create two new props: `pathModifierClassName` and `textModifierClassName` and assign `pathModifierClassName="label"` and `textModifierClassName="pie"` as default. I'm going with this one for now. * Something I'm missing? I think most of these issues could actually be simpler if we just separate `PieSlice` into `PieSlice` and `PieLabel`. Thoughts?
j285he commented 6 months ago
Poster
Collaborator

And if we are using getDisplayValueFromDatum, then shouldn't we have a getDisplayValueFromDatumLabel and a getDisplayValueFromDatumPie?

There now seems to be a lot of props that are distinguished by whether they are for the label or for the pie slices themselves (e.g. labelTextSize, pieTextSize, for consistency's sake it also makes sense to have a labelXOffset and a labelYOffset).

This goes against your idea of different types of pie slices and scalability. We can remove these props and have a uniform textSize and offset for both label and pie, at the expense of customizability. However, I think separating into two components makes more sense as they are not really types of pie slices but represent fundamentally different objects (if you get my idea).

Let me know what you think.

And if we are using `getDisplayValueFromDatum`, then shouldn't we have a `getDisplayValueFromDatumLabel` and a `getDisplayValueFromDatumPie`? There now seems to be a lot of props that are distinguished by whether they are for the label or for the pie slices themselves (e.g. `labelTextSize`, `pieTextSize`, for consistency's sake it also makes sense to have a `labelXOffset` and a `labelYOffset`). This goes against your idea of different types of pie slices and scalability. We can remove these props and have a uniform `textSize` and `offset` for both label and pie, at the expense of customizability. However, I think separating into two components makes more sense as they are not really types of pie slices but represent fundamentally different objects (if you get my idea). Let me know what you think.
a258wang commented 6 months ago
Owner

Agreed, it would probably be cleaner if we split into separate PieSlice and PieSliceLabel components or something, instead of passing in a boolean prop or a bunch of overly generic "customization" props.

Agreed, it would probably be cleaner if we split into separate `PieSlice` and `PieSliceLabel` components or something, instead of passing in a boolean prop or a bunch of overly generic "customization" props.
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
className={isLabel ? styles.labelPath : styles.piePath}
d={pathArc}
/>
<text
a258wang commented 7 months ago
Owner

same as comment about Group/g, visx has Text

same as comment about `Group`/`g`, visx has `Text`
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
/>
<text
className={isLabel ? styles.labelText : styles.pieText}
x={isLabel ? centroidX : centroidX}
a258wang commented 7 months ago
Owner

NIT: simplify to centroidX

NIT: simplify to `centroidX`
j285he marked this conversation as resolved
a258wang commented 6 months ago
Owner

Oh my goodness gitea is so buggy 😂 though it's also my fault for leaving a review in a pending state for 3 weeks 😅

Oh my goodness gitea is so buggy 😂 though it's also my fault for leaving a review in a pending state for 3 weeks 😅
j285he added 2 commits 6 months ago
continuous-integration/drone/push Build is passing
Details
7f0faa5b2e Code review
j285he added 2 commits 5 months ago
continuous-integration/drone/push Build is passing
Details
28a39696db Refactor into PieSlice and PieSliceLabel
j285he requested review from a258wang 5 months ago
a258wang approved these changes 5 months ago
a258wang left a comment
Owner

A few last comments, but otherwise looks good! We can continue to tweak things later if needed. 🙌

A few last comments, but otherwise looks good! We can continue to tweak things later if needed. 🙌
components/PieChart.tsx
/** Width of the entire graph, including labels, in pixels. */
width: number;
/** Width of the outer ring of labels, in pixels. */
labelWidth: number;
a258wang commented 5 months ago
Owner

I saw you mentioned this in the PR description, but it might be worth adding in the JSDoc comment or somewhere that the label text can get cut off if the labelWidth is too small?

Alternatively, maybe it'd make more sense to specify the radius or diameter of the pie as a prop (ie. pieRadius or pieDiameter), and then allocate labelWidth based off that, with a note that the labels might get cut off if the radius/diameter of the pie is too big relative to the width of the entire graph.

Up to you.

I saw you mentioned this in the PR description, but it might be worth adding in the JSDoc comment or somewhere that the label text can get cut off if the `labelWidth` is too small? Alternatively, maybe it'd make more sense to specify the radius or diameter of the pie as a prop (ie. `pieRadius` or `pieDiameter`), and then allocate `labelWidth` based off that, with a note that the labels might get cut off if the radius/diameter of the pie is too big relative to the width of the entire graph. Up to you.
j285he marked this conversation as resolved
components/PieChart.tsx Outdated
padRadius?: number;
/** Distance of gap in center of pie graph, in pixels */
innerRadius?: number;
/** Font size of text inside the pie, in pixels*/
a258wang commented 5 months ago
Owner

SUPER NIT: space between pixels and */

Also the inconsistency with some comments ending with periods and some not is bugging me just a little bit 😅

SUPER NIT: space between `pixels` and `*/` Also the inconsistency with some comments ending with periods and some not is bugging me just a little bit 😅
j285he marked this conversation as resolved
pages/playground.tsx Outdated
<div className={styles.page}>
<h1>Playground</h1>
<p>Show off your components here!</p>
<p>Show off your components here!</p>``
a258wang commented 5 months ago
Owner

NIT: get rid of the ``

NIT: get rid of the ``
j285he marked this conversation as resolved
j285he added 1 commit 5 months ago
continuous-integration/drone/push Build is passing
Details
321a455d02 Code review fixes
j285he added 1 commit 5 months ago
continuous-integration/drone/push Build is failing
Details
1507d29adf Merge main
j285he added 1 commit 5 months ago
continuous-integration/drone/push Build is passing
Details
0bb0341b51 Add the PieChart
j285he merged commit 9200e5f491 into main 5 months ago
j285he deleted branch j285he-pie-chart 5 months ago
j285he referenced this issue from a commit 5 months ago
Pie chart component (#19)

Reviewers

n3parikh was requested for review 7 months ago
snedadah was requested for review 7 months ago
b72zhou was requested for review 7 months ago
e26chiu was requested for review 7 months ago
a258wang approved these changes 5 months ago
continuous-integration/drone/push Build is passing
Details
The pull request has been merged as 9200e5f491.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
n3parikh
snedadah
b72zhou
e26chiu
a258wang
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#19
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'j285he-pie-chart'

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

No
Yes