Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

new(shape): support dynamic fill directly in Pie #1225

Merged
merged 4 commits into from
Jun 3, 2021
Merged

new(shape): support dynamic fill directly in Pie #1225

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d1eadc
fix(pie): support dynamic fill directly in Pie
iampueroo May 26, 2021
a0abe40
improvements from PR comments
iampueroo Jun 1, 2021
ee0a579
docs(shapes/Pie): add type description period
williaster Jun 3, 2021
14b6ecd
fix(shapes/Pie): remove extra asterisk in comment close
iampueroo Jun 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/visx-shape/src/shapes/Pie.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { arc as arcPath, pie as piePath } from '../util/D3ShapeFactories';

export type PieArcDatum<Datum> = PieArcDatumType<Datum>;

type StringAccessor<Datum> = (arc: PieArcDatum<Datum>) => string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: I chose to provide PieArcDatum since it also includes the index, but it could also be (datum: Datum, index: number). I'd like to get your thoughts on this - PieArcDatum may be unnecessary, especially if the pattern in other components is to merely provide the Datum type.

If not, I think the name shouldn't be arc but instead something like arcDatum or just pieArcDatum - I can change

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I think that PieArcDatum makes sense. Since it's strictly more info, including the computed arc size, it seems like a win to me.


export type ProvidedProps<Datum> = {
path: ArcType<$TSFIXME, PieArcDatum<Datum>>;
arcs: PieArcDatum<Datum>[];
Expand All @@ -33,6 +35,8 @@ export type PieProps<Datum> = {
pieSortValues?: PiePathConfig<Datum>['sortValues'];
/** Render function override which is passed the configured arc generator as input. */
children?: (provided: ProvidedProps<Datum>) => React.ReactNode;
/** Optional accessor function to return the fill string value of a given arc **/
williaster marked this conversation as resolved.
Show resolved Hide resolved
fill?: string | StringAccessor<Datum>;
} & Pick<PiePathConfig<Datum>, 'startAngle' | 'endAngle' | 'padAngle'> &
Pick<
ArcPathConfig<PieArcDatum<Datum>>,
Expand All @@ -56,6 +60,7 @@ export default function Pie<Datum>({
pieSortValues,
pieValue,
children,
fill = '',
...restProps
}: AddSVGProps<PieProps<Datum>, SVGPathElement>) {
const path = arcPath<PieArcDatum<Datum>>({
Expand All @@ -78,14 +83,28 @@ export default function Pie<Datum>({
// eslint-disable-next-line react/jsx-no-useless-fragment
if (children) return <>{children({ arcs, path, pie })}</>;

const fillGenerator = getFillGenerator(fill);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option that would simplify this slightly is to apply the logic inline rather than always creating the accessor. if we wanted to use the accessor multiple places I think your approach would be better but since we just use it once I think this would be okay

<path
  {...}
  fill={fill == null || typeof fill === 'string' ? fill : fill(arc)}
/>

return (
<Group className="visx-pie-arcs-group" top={top} left={left}>
{arcs.map((arc, i) => (
<g key={`pie-arc-${i}`}>
<path className={cx('visx-pie-arc', className)} d={path(arc) || ''} {...restProps} />
<path
className={cx('visx-pie-arc', className)}
d={path(arc) || ''}
fill={fillGenerator(arc)}
{...restProps}
/>
{centroid?.(path.centroid(arc), arc)}
</g>
))}
</Group>
);
}

function getFillGenerator<Datum>(fill: string | StringAccessor<Datum>): StringAccessor<Datum> {
williaster marked this conversation as resolved.
Show resolved Hide resolved
if (typeof fill === 'string') {
// Convert constant fill string to constant fill generator function
return (arc: PieArcDatum<Datum>) => fill;
}
return fill;
}
21 changes: 20 additions & 1 deletion packages/visx-shape/test/Pie.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import { shallow } from 'enzyme';

import { Pie } from '../src';
import { PieProps } from '../src/shapes/Pie';
import { PieArcDatum, PieProps } from '../src/shapes/Pie';

interface Datum {
date: string;
Expand All @@ -14,6 +14,7 @@ interface Datum {
Opera: string;
Mozilla: string;
'Other/Unknown': string;
color: string;
}

const browserUsage: Datum[] = [
Expand All @@ -27,6 +28,7 @@ const browserUsage: Datum[] = [
Opera: '1.32',
Mozilla: '0.12',
'Other/Unknown': '0.01',
color: 'blue',
},
{
date: '2015 Jun 16',
Expand All @@ -38,6 +40,7 @@ const browserUsage: Datum[] = [
Opera: '1.32',
Mozilla: '0.12',
'Other/Unknown': '0.01',
color: 'red',
},
];

Expand Down Expand Up @@ -102,6 +105,22 @@ describe('<Pie />', () => {
expect(fn).toHaveBeenCalled();
});

test('it should accept a custom fill function', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding tests!

const paths = PieWrapper({
fill: (datum: PieArcDatum<Datum>) => datum.data.color,
}).find('path');
expect(paths.at(0).prop('fill')).toBe('blue');
expect(paths.at(1).prop('fill')).toBe('red');
});

test('it should accept a constant string fill value', () => {
const paths = PieWrapper({
fill: 'purple',
}).find('path');
expect(paths.at(0).prop('fill')).toBe('purple');
expect(paths.at(1).prop('fill')).toBe('purple');
});

test('it should call children function with { arcs, path, pie }', () => {
const fn = jest.fn();
PieChildren({ children: fn });
Expand Down