Pie chart component #19
|
@ -0,0 +1,26 @@
|
|||
.piePath {
|
||||
fill: var(--tertiary-background);
|
||||
}
|
||||
|
||||
.labelPath {
|
||||
fill-opacity: 0;
|
||||
}
|
||||
j285he marked this conversation as resolved
|
||||
|
||||
.pieText,
|
||||
.labelText {
|
||||
fill: var(--label);
|
||||
font-weight: 800;
|
||||
}
|
||||
|
||||
j285he marked this conversation as resolved
a258wang
commented
NITs:
NITs:
- `calc(40 rem / 16)`
- should this be font-weight 700 (bold)?
a258wang
commented
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
a258wang
commented
NIT: NIT: `0 0 calc(6rem / 16) var(--primary-accent)`
|
||||
.group:hover .pieText {
|
||||
display: inline;
|
||||
}
|
|
@ -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
a258wang
commented
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 🙂 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
a258wang
commented
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 Alternatively, maybe it'd make more sense to specify the radius or diameter of the pie as a prop (ie. 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
a258wang
commented
SUPER NIT: space between 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
a258wang
commented
NIT: include 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
a258wang
commented
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
a258wang
commented
NIT: maybe name this component 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
a258wang
commented
Visx has a (My guess is that visx components need to be inside an (My second guess is because there's no visx equivalent of the 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
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
a258wang
commented
I noticed that we have a lot of styles etc. that are conditional on the
Numeric offsets like the
My main qualm about making everything conditional on 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
I fixed the For We will still have conditionals in places like
Is this ok? Also, this approach means we want a "default behavior" ( However, the text inside the pie actually needs to be hidden, which means either:
I think most of these issues could actually be simpler if we just separate 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
And if we are using 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. This goes against your idea of different types of pie slices and scalability. We can remove these props and have a uniform 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
Agreed, it would probably be cleaner if we split into separate 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
a258wang
commented
same as comment about same as comment about `Group`/`g`, visx has `Text`
|
||||
pieTextYOffset={pieTextYOffset}
|
||||
getPieDisplayValueFromDatum={getPieDisplayValueFromDatum}
|
||||
j285he marked this conversation as resolved
Outdated
a258wang
commented
NIT: simplify to 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>
|
||||
);
|
||||
})}
|
||||
</>
|
||||
);
|
||||
}
|
|
@ -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 },
|
||||
|
|
|
@ -5,7 +5,6 @@
|
|||
"requires": true,
|
||||
"packages": {
|
||||
"": {
|
||||
"name": "cs-2022-class-profile",
|
||||
"version": "0.1.0",
|
||||
"dependencies": {
|
||||
"@visx/axis": "^2.10.0",
|
||||
|
|
|
@ -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>
|
||||
|
|
Just curious, does
fill
matter iffill-opacity
is set to 0?