Pie chart component #19

Merged
j285he merged 23 commits from j285he-pie-chart into main 2022-09-07 22:20:31 -04:00
5 changed files with 229 additions and 4 deletions

View File

@ -0,0 +1,26 @@
.piePath {
fill: var(--tertiary-background);
}
.labelPath {
fill-opacity: 0;
}
j285he marked this conversation as resolved
Review

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

Just curious, does `fill` matter if `fill-opacity` is set to 0?
.pieText,
.labelText {
fill: var(--label);
font-weight: 800;
}
j285he marked this conversation as resolved
Review

NITs:

  • calc(40 rem / 16)
  • should this be font-weight 700 (bold)?
NITs: - `calc(40 rem / 16)` - should this be font-weight 700 (bold)?
Review

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.
.pieText {
display: none;
}
.group:hover > .piePath {
fill: var(--primary-accent);
filter: drop-shadow(0px 0px calc(6rem / 16) var(--primary-accent));
}
j285he marked this conversation as resolved
Review

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

NIT: `0 0 calc(6rem / 16) var(--primary-accent)`
.group:hover .pieText {
display: inline;
}

180
components/PieChart.tsx Normal file
View File

@ -0,0 +1,180 @@
import { Group } from "@visx/group";
import Pie, { ProvidedProps } from "@visx/shape/lib/shapes/Pie";
import { Text } from "@visx/text";
import React from "react";
import styles from "./PieChart.module.css";
interface PieChartProps {
data: PieChartData[];
/** Width of the entire graph, including labels, in pixels. */
width: number;
/** Width of the outer ring of labels, in pixels. Label text may be cut off if specified value is too small. */
j285he marked this conversation as resolved Outdated

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 🙂
labelWidth: number;
j285he marked this conversation as resolved
Review

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.
/** Distance between pie slices, in pixels. */
padRadius?: number;
/** Distance of gap in center of pie graph, in pixels. */
innerRadius?: number;
/** Font size of text inside the pie, in pixels. */
j285he marked this conversation as resolved Outdated

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 😅
pieTextSize?: number;
/** X-axis offset of the pie text, in pixels. */
pieTextXOffset?: number;
/** Y-axis offset of the pie text, in pixels. */
pieTextYOffset?: number;
/** Accessor function to get value to display as pie text from datum. */
getPieDisplayValueFromDatum?: (datum: PieChartData) => string;
/** Font size of labels outside the pie, in pixels. */
labelTextSize?: number;
/** X-axis offset of the label text, in pixels. */
labelTextXOffset?: number;
/** Y-axis offset of the label text, in pixels. */
j285he marked this conversation as resolved Outdated

NIT: include className when destructuring the other props above

NIT: include `className` when destructuring the other props above
labelTextYOffset?: number;
/** Accessor function to get value to display as label text from datum. */
getLabelDisplayValueFromDatum?: (datum: PieChartData) => string;
j285he marked this conversation as resolved Outdated

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.
className?: string;
}
interface PieChartData {
category: string;
value: number;
}
export function PieChart({
data,
width,
labelWidth,
padRadius = width * 0.35,
innerRadius = width * 0.015,
pieTextSize = 40,
pieTextXOffset = 0,
pieTextYOffset = 10,
getPieDisplayValueFromDatum = (datum: PieChartData) => `${datum.value}%`,
labelTextSize = 40,
labelTextXOffset = 0,
labelTextYOffset = 0,
getLabelDisplayValueFromDatum = (datum: PieChartData) => `${datum.category}`,
className,
}: PieChartProps) {
const pieWidth = width * 0.5 - labelWidth;
return (
<svg className={className} width={width} height={width}>
j285he marked this conversation as resolved Outdated

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?
<Group top={width * 0.5} left={width * 0.5}>
<Pie
data={data}
pieValue={(d: PieChartData) => d.value}
cornerRadius={10}
padAngle={0.075}
padRadius={padRadius}
innerRadius={innerRadius}
j285he marked this conversation as resolved Outdated

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?`

Third option: I forgot about Group 😅

Third option: I forgot about Group 😅
outerRadius={pieWidth}
>
{(pie) => (
<PieSlice
{...pie}
pieTextSize={pieTextSize}
j285he marked this conversation as resolved Outdated

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. 🙂

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?

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.

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.
pieTextXOffset={pieTextXOffset}
j285he marked this conversation as resolved Outdated

same as comment about Group/g, visx has Text

same as comment about `Group`/`g`, visx has `Text`
pieTextYOffset={pieTextYOffset}
getPieDisplayValueFromDatum={getPieDisplayValueFromDatum}
j285he marked this conversation as resolved Outdated

NIT: simplify to centroidX

NIT: simplify to `centroidX`
/>
)}
</Pie>
<Pie
data={data}
pieValue={(d: PieChartData) => d.value}
innerRadius={pieWidth}
outerRadius={width * 0.5}
>
{(pie) => (
<PieSliceLabel
{...pie}
labelTextSize={labelTextSize}
labelTextXOffset={labelTextXOffset}
labelTextYOffset={labelTextYOffset}
getLabelDisplayValueFromDatum={getLabelDisplayValueFromDatum}
/>
)}
</Pie>
</Group>
</svg>
);
}
type PieSliceProps<PieChartData> = ProvidedProps<PieChartData> & {
pieTextSize: number;
pieTextXOffset: number;
pieTextYOffset: number;
getPieDisplayValueFromDatum: (datum: PieChartData) => string;
};
export function PieSlice({
path,
arcs,
pieTextSize,
pieTextXOffset,
pieTextYOffset,
getPieDisplayValueFromDatum,
}: PieSliceProps<PieChartData>) {
return (
<>
{arcs.map((arc) => {
const [centroidX, centroidY] = path.centroid(arc);
const pathArc = path(arc) as string;
return (
<Group className={styles.group} key={`arc-${arc.data.category}`}>
<path className={styles.piePath} d={pathArc} />
<Text
className={styles.pieText}
x={centroidX + pieTextXOffset}
y={centroidY + pieTextYOffset}
textAnchor="middle"
fontSize={pieTextSize}
>
{`${getPieDisplayValueFromDatum(arc.data)}`}
</Text>
</Group>
);
})}
</>
);
}
type PieSliceLabelProps<PieChartData> = ProvidedProps<PieChartData> & {
labelTextSize: number;
labelTextXOffset: number;
labelTextYOffset: number;
getLabelDisplayValueFromDatum: (datum: PieChartData) => string;
};
export function PieSliceLabel({
path,
arcs,
labelTextSize,
labelTextXOffset,
labelTextYOffset,
getLabelDisplayValueFromDatum,
}: PieSliceLabelProps<PieChartData>) {
return (
<>
{arcs.map((arc) => {
const [centroidX, centroidY] = path.centroid(arc);
const pathArc = path(arc) as string;
return (
<Group className={styles.group} key={`arc-${arc.data.category}`}>
<path className={styles.labelPath} d={pathArc} />
<Text
className={styles.labelText}
x={centroidX + labelTextXOffset}
y={centroidY + labelTextYOffset}
textAnchor="middle"
fontSize={labelTextSize}
>
{`${getLabelDisplayValueFromDatum(arc.data)}`}
</Text>
</Group>
);
})}
</>
);
}

View File

@ -36,6 +36,21 @@ export const mockCategoricalData = [
},
];
export const mockPieData = [
{
category: "Nightingale",
value: 42,
},
{
category: "Quail",
value: 48,
},
{
category: "Cuckoo",
value: 10,
},
];
export const moreMockCategoricalData = [
{ key: "Python", value: 29.53 },
{ key: "Java", value: 17.06 },

1
package-lock.json generated
View File

@ -5,7 +5,6 @@
"requires": true,
"packages": {
"": {
"name": "cs-2022-class-profile",
"version": "0.1.0",
"dependencies": {
"@visx/axis": "^2.10.0",

View File

@ -6,9 +6,11 @@ import {
mockBoxPlotData,
mockQuoteData,
mockQuoteDataLong,
mockPieData,
} from "data/mocks";
import React from "react";
import { PieChart } from "@/components/PieChart";
import { QuotationCarousel } from "@/components/QuotationCarousel";
import { ColorPalette } from "../components/ColorPalette";
@ -21,8 +23,13 @@ export default function Home() {
<div className={styles.page}>
<h1>Playground</h1>
<p>Show off your components here!</p>
<h2>
<code>{"<PieChart />"}</code>
</h2>
<div style={{ padding: "30px" }}>
<PieChart data={mockPieData} width={800} labelWidth={215} />
</div>
<ColorPalette />
<h2>
<code>{"<BarGraphHorizontal />"}</code>
</h2>
@ -38,7 +45,6 @@ export default function Home() {
right: 20,
}}
/>
<h2>
<code>{"<BarGraphVertical />"}</code>
</h2>
@ -58,7 +64,6 @@ export default function Home() {
right: 20,
}}
/>
<h2>
<code>{"<WordCloud />"}</code>
</h2>