-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Extract FocusedPieArc from PieArcPlot
#20613
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.
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
Conversation
|
Deploy preview: https://deploy-preview-20613--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
|
Wrong PR title 😄 |
FocusedPieArc from PieArcPlot
CodSpeed Performance ReportMerging #20613 will not alter performanceComparing Summary
Footnotes |
| 'x-charts/src/BarChart/AnimatedBarElement.tsx', | ||
| 'x-charts/src/RadarChart/RadarDataProvider/RadarDataProvider.tsx', | ||
| 'x-charts/src/LineChart/FocusedLineMark.tsx', | ||
| 'x-charts/src/PieChart/FocusedPieArc.tsx', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this technically documented in the accessibility docs? Should we add it there instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff will create a cross link between docs pages and the component API page.
Those components API are empty. They are just here to be display, so I prefer avoiding adding noise
| { "name": "UsePieSeriesContextReturnValue", "kind": "TypeAlias" }, | ||
| { "name": "UsePieSeriesReturnValue", "kind": "TypeAlias" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs to be re-generated
| /> | ||
| ))} | ||
| {/* Render the focus indicator last, so it can align nicely over all arcs */} | ||
| {focusedItem && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this technically a breaking change? Someone using PieArcPlot will now be missing the outline, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const isFaded = !isHighlighted && isItemFaded(currentItem); | ||
| const isFocused = isItemFocused({ seriesType: 'pie', seriesId, dataIndex: itemIndex }); | ||
|
|
||
| // TODO v9: Replace the second argument with the result of useSeriesLayout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simplified version to illustrate the issue
export fucntion PiePlot(){
const innerRadius = usePieSeriesLayout()
return <g>
<PieArcPlot innerRadius={innerRadius} /> // This component calls useTransformData()
<PieArcLabelPlot innerRadius={innerRadius} /> // This component calls useTransformData()
</g>
}The issue is that PiePlot, PieArcPlot, and PieArcLabelPlot are all exported.
If I use directly the result of usePieSeriesLayout() in the useTransformData() the innerRadius prop would have no effect which is a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how we would fix this in the future then 🤔
Like, shouldn't we default to the value of pieSeriesLayout to prevent inconsistency?
const pieSeriesLayout = usePieSeriesLayout();
const layout = pieSeriesLayout[seriesId]
const arcSizes = applyArcModifiers(
series,
{
radius: {
inner: series.innerRadius ?? layout.radius.innerRadius
...
}
},
isHighlighted,
isFaded,
);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we could
- remove the
PiePlot - remove
innerRadiusprops fromPieArcPlotto only use the seriesLayout
Then we will be able to do
const pieSeriesLayout = usePieSeriesLayout();
const layout = pieSeriesLayout[seriesId]
const arcSizes = applyArcModifiers(
series,
layout,
isHighlighted,
isFaded,
);By the way series.innerRadius ?? layout.radius.innerRadius is nto correct, because :
series.innerRadiusis the user config. It can be a percentage like"90%"layout.radius.innerRadiusis the equivalent in px of theseries.innerRadius
The only thing that interest us from series is the series.faded.inneRadius and series.highlighted.inneRadius
| /** | ||
| * Function that returns arc properties after applying transformation from highlight/fade states. | ||
| */ | ||
| export function applyArcModifiers( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"apply" makes it feel like this function is mutable, but it just returns the modifiers, so I'd suggest calling it getArcModifiers or getPieArcModifiers if you want to be more explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it does not get the modifiers, it applies the focused and highlighted objects when relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could call it getArcProperties instead or getModifiedArcProperties, for example. Not a blocker, though
| DefaultizedPieSeriesType, | ||
| 'cornerRadius' | 'paddingAngle' | 'id' | 'highlighted' | 'faded' | 'data' | ||
| >, | ||
| seriesLayout: Pick<PieSeriesLayout, 'radius'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're just picking the radius, why not simplify the type?
| seriesLayout: Pick<PieSeriesLayout, 'radius'>, | |
| layoutRadius: PieSeriesLayout['radius'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easier to have a split that match hooks.
seriesDefis a subset of theuseSeriesseriesLayoutis a subset of theuseSeriesLayout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually prefer simpler types so that functions are easier to test. IMO, the called function doesn't need to know where the data comes from, that's the calling function's responsibility. Not a big deal, though, just style preference

Follow up on top of #20611