new(shape): support dynamic fill directly in Pie #1225
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
williaster
changed the title
There was a problem hiding this comment.
Thanks for the great addition @iampueroo! This looks good to me overall, I had one potential simplification (see what you think) but otherwise we can get this in!
| @@ -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); | |||
There was a problem hiding this comment.
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)}
/>| @@ -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; | |||
There was a problem hiding this comment.
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.
| @@ -102,6 +105,22 @@ describe('<Pie />', () => { | |||
| expect(fn).toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
| test('it should accept a custom fill function', () => { | |||
There was a problem hiding this comment.
thanks for adding tests!
|
@williaster thanks for the review. I just pushed this commit which removes the fill generator function and changes a variable name to be clearer. Happy to keep tweaking things as needed. |
|
🎉 This PR is included in version |
This is a PR for issue #1193
💥 Breaking Changes
fillproperty supported onlystringvalue. It still accepts thestringvalue (but can also accept a string accessor function). I added a test case for both.🚀 Enhancements
<Pie />component. Prior, they had to add the<path>elements as children of the<Pie>in order to dynamic colors.📝 Documentation
fillfunction can now be astringor a function that receives anPieArcDatum<Datum>and returns astringfill value:(arc: PieArcDatum<Datum>) => string.🐛 Bug Fix
🏠 Internal