Pie chart component #19
Labels
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
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#19
Loading…
Reference in New Issue
No description provided.
Delete Branch "j285he-pie-chart"
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?
Closes #5.
To fix:
Inner pie slice text not perfectly centered
Create optional props for
padRadius
andinnerRadius
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
This is what the pie looks like hovered on a small slice.
WIP: Pie chart componentto Pie chart componentI 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.
If you could investigate and come up with a fix, that'd be great! Let me know if you want any help though 🙂
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;
Just curious, does
fill
matter iffill-opacity
is set to 0?@ -0,0 +11,4 @@
.labelText {
fill: var(--label);
font-size: 40px;
font-weight: 800;
NITs:
calc(40 rem / 16)
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.
@ -0,0 +20,4 @@
.group:hover > .piePath {
fill: var(--primary-accent);
filter: drop-shadow(0px 0px 6px var(--primary-accent));
NIT:
0 0 calc(6rem / 16) var(--primary-accent)
@ -0,0 +9,4 @@
width: number;
labelWidth: number;
padRadius?: number;
innerRadius?: number;
Suggestion: Add JSDoc comments above the less obvious props to describe what they do, like this:
This will be helpful for people using your component in the future, since VSCode can detect this information and display it on hover 🙂
@ -0,0 +27,4 @@
}: PieChartProps) {
const pieWidth = width * 0.5 - labelWidth;
return (
<svg className={props.className} width={width} height={width}>
NIT: include
className
when destructuring the other props above@ -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}
NIT: same thing, destructuring
In general, my opionion is to either destructure all the props for the component, or none of them.
@ -0,0 +57,4 @@
isLabel: boolean;
};
export function PieSlices({ isLabel, ...props }: PieSlicesProps<PieChartData>) {
NIT: maybe name this component
PieSlice
since it's just one slice of the pie?@ -0,0 +65,4 @@
const pathArc = props.path(arc) as string;
return (
<g className={styles.group} key={`arc-${arc.data.category}`}>
Visx has a
Group
component which eventually renders ag
element, is there a reason why we're writing raw SVG stuff here instead of using theGroup
?(My guess is that visx components need to be inside an
svg
, though apparently it's fine to nestsvg
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?`Third option: I forgot about Group 😅
@ -0,0 +71,4 @@
d={pathArc}
/>
<text
className={isLabel ? styles.labelText : styles.pieText}
I noticed that we have a lot of styles etc. that are conditional on the
isLabel
prop. Have we considered passing inmodifierClassName
as a prop, and use CSS selectors to apply the styles without branching on a boolean all the time? eg.Numeric offsets like the
+10
incentroidY + 10
could also be passed in as a prop. Even the displayed text value likearc.data.category
could be obtained using a function that is passed in as a prop: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 fixed the
centroidY
issue.For
isLabel
I'm running into a few issues.We will still have conditionals in places like
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:
.path
and.text
behavior for the inner pie to be default. This will be unintuitive as text will be hidden by default..path
and.text
behavior for the label to be default. However, label path is hidden by default..path
behavior default and label's.text
behavior default, and create two new props:pathModifierClassName
andtextModifierClassName
and assignpathModifierClassName="label"
andtextModifierClassName="pie"
as default. I'm going with this one for now.I think most of these issues could actually be simpler if we just separate
PieSlice
intoPieSlice
andPieLabel
. Thoughts?And if we are using
getDisplayValueFromDatum
, then shouldn't we have agetDisplayValueFromDatumLabel
and agetDisplayValueFromDatumPie
?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 alabelXOffset
and alabelYOffset
).This goes against your idea of different types of pie slices and scalability. We can remove these props and have a uniform
textSize
andoffset
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.
Agreed, it would probably be cleaner if we split into separate
PieSlice
andPieSliceLabel
components or something, instead of passing in a boolean prop or a bunch of overly generic "customization" props.@ -0,0 +72,4 @@
className={isLabel ? styles.labelPath : styles.piePath}
d={pathArc}
/>
<text
same as comment about
Group
/g
, visx hasText
@ -0,0 +74,4 @@
/>
<text
className={isLabel ? styles.labelText : styles.pieText}
x={isLabel ? centroidX : centroidX}
NIT: simplify to
centroidX
Oh my goodness gitea is so buggy 😂 though it's also my fault for leaving a review in a pending state for 3 weeks 😅
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;
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
orpieDiameter
), and then allocatelabelWidth
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.
@ -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*/
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 😅
@ -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>``
NIT: get rid of the ``