Pie chart component #19

Merged
j285he merged 23 commits from j285he-pie-chart into main 2022-09-07 22:20:31 -04:00
Member

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 2022-06-18 22:55:03 -04:00
j285he added 1 commit 2022-06-18 22:55:12 -04:00
Author
Member

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

This is what the pie looks like hovered on a small slice.
j285he closed this pull request 2022-06-18 23:06:32 -04:00
j285he reopened this pull request 2022-06-18 23:08:36 -04:00
j285he added 4 commits 2022-07-06 19:56:15 -04:00
j285he changed title from WIP: Pie chart component to Pie chart component 2022-07-06 19:56:21 -04:00
j285he requested review from n3parikh 2022-07-06 21:19:10 -04:00
j285he requested review from a258wang 2022-07-06 21:19:15 -04:00
j285he requested review from snedadah 2022-07-06 21:19:19 -04:00
j285he requested review from b72zhou 2022-07-06 21:19:23 -04:00
j285he requested review from e26chiu 2022-07-06 21:19:30 -04:00
j285he added 2 commits 2022-07-13 20:07:20 -04:00
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 🙂
j285he added 3 commits 2022-07-27 23:27:17 -04:00
continuous-integration/drone/push Build is passing Details
6735c52914
Bar Graph Component (#16)
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

Co-authored-by: Amy Wang <a258wang@csclub.uwaterloo.ca>
Reviewed-on: #16
Reviewed-by: Shahan Neda <snedadah@csclub.uwaterloo.ca>
continuous-integration/drone/push Build is passing Details
fc3512f72f
Fix shadow not working
a258wang requested changes 2022-08-03 23:51:17 -04:00
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 😞
@ -0,0 +4,4 @@
.labelPath {
fill: var(--primary-background);
fill-opacity: 0;
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
@ -0,0 +11,4 @@
.labelText {
fill: var(--label);
font-size: 40px;
font-weight: 800;
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)?
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
@ -0,0 +20,4 @@
.group:hover > .piePath {
fill: var(--primary-accent);
filter: drop-shadow(0px 0px 6px var(--primary-accent));
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
@ -0,0 +9,4 @@
width: number;
labelWidth: number;
padRadius?: number;
innerRadius?: number;
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
@ -0,0 +27,4 @@
}: PieChartProps) {
const pieWidth = width * 0.5 - labelWidth;
return (
<svg className={props.className} width={width} height={width}>
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
@ -0,0 +30,4 @@
<svg className={props.className} width={width} height={width}>
<Group top={width * 0.5} left={width * 0.5}>
<Pie
data={props.data}
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
@ -0,0 +57,4 @@
isLabel: boolean;
};
export function PieSlices({ isLabel, ...props }: PieSlicesProps<PieChartData>) {
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
@ -0,0 +65,4 @@
const pathArc = props.path(arc) as string;
return (
<g className={styles.group} key={`arc-${arc.data.category}`}>
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?`
Author
Member

Third option: I forgot about Group 😅

Third option: I forgot about Group 😅
j285he marked this conversation as resolved
@ -0,0 +71,4 @@
d={pathArc}
/>
<text
className={isLabel ? styles.labelText : styles.pieText}
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. 🙂
Author
Member

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?
Author
Member

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.
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
@ -0,0 +72,4 @@
className={isLabel ? styles.labelPath : styles.piePath}
d={pathArc}
/>
<text
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
@ -0,0 +74,4 @@
/>
<text
className={isLabel ? styles.labelText : styles.pieText}
x={isLabel ? centroidX : centroidX}
Owner

NIT: simplify to centroidX

NIT: simplify to `centroidX`
j285he marked this conversation as resolved
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 2022-08-09 18:00:31 -04:00
continuous-integration/drone/push Build is passing Details
7f0faa5b2e
Code review
j285he added 2 commits 2022-08-31 20:13:26 -04:00
j285he requested review from a258wang 2022-08-31 20:18:54 -04:00
a258wang approved these changes 2022-09-01 23:45:47 -04:00
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. 🙌
@ -0,0 +10,4 @@
/** Width of the entire graph, including labels, in pixels. */
width: number;
/** Width of the outer ring of labels, in pixels. */
labelWidth: number;
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
@ -0,0 +15,4 @@
padRadius?: number;
/** Distance of gap in center of pie graph, in pixels */
innerRadius?: number;
/** Font size of text inside the pie, in pixels*/
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
@ -12,3 +18,3 @@
<div className={styles.page}>
<h1>Playground</h1>
<p>Show off your components here!</p>
<p>Show off your components here!</p>``
Owner

NIT: get rid of the ``

NIT: get rid of the ``
j285he marked this conversation as resolved
j285he added 1 commit 2022-09-07 20:49:18 -04:00
continuous-integration/drone/push Build is passing Details
321a455d02
Code review fixes
j285he added 1 commit 2022-09-07 22:03:31 -04:00
continuous-integration/drone/push Build is failing Details
1507d29adf
Merge main
j285he added 1 commit 2022-09-07 22:07:22 -04:00
continuous-integration/drone/push Build is passing Details
0bb0341b51
Add the PieChart
j285he merged commit 9200e5f491 into main 2022-09-07 22:20:31 -04:00
j285he deleted branch j285he-pie-chart 2022-09-07 22:20:31 -04:00
j285he referenced this issue from a commit 2022-09-07 22:20:31 -04:00
Sign in to join this conversation.
No Milestone
No project
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#19
No description provided.