-
-
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
Changes from all commits
bda6815
e490b80
4a3e2f8
61823f3
218e9b0
25ce936
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| 'use client'; | ||
| import * as React from 'react'; | ||
| import { useTheme } from '@mui/material/styles'; | ||
| import { useFocusedItem } from '../hooks/useFocusedItem'; | ||
| import { usePieSeriesContext, usePieSeriesLayout } from '../hooks/usePieSeries'; | ||
| import { PieArc, pieArcClasses, type PieArcProps } from './PieArc'; | ||
| import { useItemHighlighted } from '../hooks/useItemHighlighted'; | ||
| import { getModifiedArcProperties } from './dataTransform/getModifiedArcProperties'; | ||
|
|
||
| export function FocusedPieArc( | ||
| props: Partial< | ||
| Omit< | ||
| PieArcProps, | ||
| 'startAngle' | 'endAngle' | 'id' | 'dataIndex' | 'isFaded' | 'isHighlighted' | 'isFocused' | ||
| > | ||
| >, | ||
| ) { | ||
| const theme = useTheme(); | ||
| const focusedItem = useFocusedItem(); | ||
| const pieSeriesLayout = usePieSeriesLayout(); | ||
|
|
||
| const { isHighlighted, isFaded } = useItemHighlighted(focusedItem); | ||
| const pieSeries = usePieSeriesContext(); | ||
|
|
||
| if (focusedItem === null || focusedItem.seriesType !== 'pie' || !pieSeries) { | ||
| return null; | ||
| } | ||
|
|
||
| const series = pieSeries?.series[focusedItem.seriesId]; | ||
| const { center, radius } = pieSeriesLayout[focusedItem.seriesId]; | ||
|
|
||
| if (!series || !center || !radius) { | ||
| return null; | ||
| } | ||
|
|
||
| const item = series.data[focusedItem.dataIndex]; | ||
|
|
||
| const arcSizes = getModifiedArcProperties( | ||
| series, | ||
| pieSeriesLayout[focusedItem.seriesId], | ||
| isHighlighted, | ||
| isFaded, | ||
| ); | ||
|
|
||
| return ( | ||
| <PieArc | ||
| transform={`translate(${pieSeriesLayout[series.id].center.x}, ${pieSeriesLayout[series.id].center.y})`} | ||
| startAngle={item.startAngle} | ||
| endAngle={item.endAngle} | ||
| color="transparent" | ||
| pointerEvents="none" | ||
| skipInteraction | ||
| skipAnimation | ||
| stroke={(theme.vars ?? theme).palette.text.primary} | ||
| id={series.id} | ||
| className={pieArcClasses.focusIndicator} | ||
| dataIndex={focusedItem.dataIndex} | ||
| isFaded={false} | ||
| isHighlighted={false} | ||
| isFocused={false} | ||
| strokeWidth={3} | ||
| {...arcSizes} | ||
| {...props} | ||
| /> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,50 @@ | ||||||
| import type { DefaultizedPieSeriesType, PieSeriesLayout } from '../../models/seriesType/pie'; | ||||||
| import { deg2rad } from '../../internals/angleConversion'; | ||||||
|
|
||||||
| /** | ||||||
| * Function that returns arc properties after applying transformation from highlight/fade states. | ||||||
| */ | ||||||
| export function getModifiedArcProperties( | ||||||
| seriesDef: Pick< | ||||||
| DefaultizedPieSeriesType, | ||||||
| 'cornerRadius' | 'paddingAngle' | 'id' | 'highlighted' | 'faded' | 'data' | ||||||
| >, | ||||||
| seriesLayout: Pick<PieSeriesLayout, 'radius'>, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're just picking the radius, why not simplify the type?
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's easier to have a split that match hooks.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
| isHighlighted: boolean, | ||||||
| isFaded: boolean, | ||||||
| ) { | ||||||
| const { | ||||||
| faded, | ||||||
| highlighted, | ||||||
| paddingAngle: basePaddingAngle = 0, | ||||||
| cornerRadius: baseCornerRadius = 0, | ||||||
| } = seriesDef; | ||||||
|
|
||||||
| const { | ||||||
| radius: { inner: baseInnerRadius = 0, label: baseArcLabelRadius, outer: baseOuterRadius }, | ||||||
| } = seriesLayout; | ||||||
|
|
||||||
| const attributesOverride = { | ||||||
| additionalRadius: 0, | ||||||
| ...((isFaded && faded) || (isHighlighted && highlighted) || {}), | ||||||
| }; | ||||||
| const paddingAngle = Math.max(0, deg2rad(attributesOverride.paddingAngle ?? basePaddingAngle)); | ||||||
| const innerRadius = Math.max(0, attributesOverride.innerRadius ?? baseInnerRadius); | ||||||
|
|
||||||
| const outerRadius = Math.max( | ||||||
| 0, | ||||||
| attributesOverride.outerRadius ?? baseOuterRadius + attributesOverride.additionalRadius, | ||||||
| ); | ||||||
| const cornerRadius = attributesOverride.cornerRadius ?? baseCornerRadius; | ||||||
|
|
||||||
| const arcLabelRadius = | ||||||
| attributesOverride.arcLabelRadius ?? baseArcLabelRadius ?? (innerRadius + outerRadius) / 2; | ||||||
|
|
||||||
| return { | ||||||
| paddingAngle, | ||||||
| innerRadius, | ||||||
| outerRadius, | ||||||
| cornerRadius, | ||||||
| arcLabelRadius, | ||||||
| }; | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ import { | |
| } from '../../models/seriesType/pie'; | ||
| import { useItemHighlightedGetter } from '../../hooks/useItemHighlightedGetter'; | ||
| import { useIsItemFocusedGetter } from '../../hooks/useIsItemFocusedGetter'; | ||
| import { deg2rad } from '../../internals/angleConversion'; | ||
| import { getModifiedArcProperties } from './getModifiedArcProperties'; | ||
|
|
||
| export interface AnimatedObject { | ||
| innerRadius: number; | ||
|
|
@@ -33,17 +33,7 @@ export function useTransformData( | |
| > & | ||
| ComputedPieRadius, | ||
| ) { | ||
| const { | ||
| id: seriesId, | ||
| data, | ||
| faded, | ||
| highlighted, | ||
| paddingAngle: basePaddingAngle = 0, | ||
| innerRadius: baseInnerRadius = 0, | ||
| arcLabelRadius: baseArcLabelRadius, | ||
| outerRadius: baseOuterRadius, | ||
| cornerRadius: baseCornerRadius = 0, | ||
| } = series; | ||
| const { id: seriesId, data, faded, highlighted } = series; | ||
|
|
||
| const { isFaded: isItemFaded, isHighlighted: isItemHighlighted } = useItemHighlightedGetter(); | ||
| const isItemFocused = useIsItemFocusedGetter(); | ||
|
|
@@ -59,26 +49,24 @@ export function useTransformData( | |
| const isFaded = !isHighlighted && isItemFaded(currentItem); | ||
| const isFocused = isItemFocused({ seriesType: 'pie', seriesId, dataIndex: itemIndex }); | ||
|
|
||
| // TODO v9: Replace the second argument with the result of useSeriesLayout | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we use it now?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 If I use directly the result of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 const pieSeriesLayout = usePieSeriesLayout();
const layout = pieSeriesLayout[seriesId]
const arcSizes = applyArcModifiers(
series,
{
radius: {
inner: series.innerRadius ?? layout.radius.innerRadius
...
}
},
isHighlighted,
isFaded,
);
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the future, we could
Then we will be able to do const pieSeriesLayout = usePieSeriesLayout();
const layout = pieSeriesLayout[seriesId]
const arcSizes = applyArcModifiers(
series,
layout,
isHighlighted,
isFaded,
);By the way
The only thing that interest us from |
||
| const arcSizes = getModifiedArcProperties( | ||
| series, | ||
| { | ||
| radius: { | ||
| inner: series.innerRadius ?? 0, | ||
| outer: series.outerRadius, | ||
| label: series.arcLabelRadius ?? 0, | ||
| available: 0, | ||
| }, | ||
| }, | ||
| isHighlighted, | ||
| isFaded, | ||
| ); | ||
| const attributesOverride = { | ||
| additionalRadius: 0, | ||
| ...((isFaded && faded) || (isHighlighted && highlighted) || {}), | ||
| }; | ||
| const paddingAngle = Math.max( | ||
| 0, | ||
| deg2rad(attributesOverride.paddingAngle ?? basePaddingAngle), | ||
| ); | ||
| const innerRadius = Math.max(0, attributesOverride.innerRadius ?? baseInnerRadius); | ||
|
|
||
| const outerRadius = Math.max( | ||
| 0, | ||
| attributesOverride.outerRadius ?? baseOuterRadius + attributesOverride.additionalRadius, | ||
| ); | ||
| const cornerRadius = attributesOverride.cornerRadius ?? baseCornerRadius; | ||
|
|
||
| const arcLabelRadius = | ||
| attributesOverride.arcLabelRadius ?? | ||
| baseArcLabelRadius ?? | ||
| (innerRadius + outerRadius) / 2; | ||
|
|
||
| return { | ||
| ...item, | ||
|
|
@@ -87,27 +75,10 @@ export function useTransformData( | |
| isFaded, | ||
| isHighlighted, | ||
| isFocused, | ||
| paddingAngle, | ||
| innerRadius, | ||
| outerRadius, | ||
| cornerRadius, | ||
| arcLabelRadius, | ||
| ...arcSizes, | ||
| }; | ||
| }), | ||
| [ | ||
| baseCornerRadius, | ||
| baseInnerRadius, | ||
| baseOuterRadius, | ||
| basePaddingAngle, | ||
| baseArcLabelRadius, | ||
| data, | ||
| faded, | ||
| highlighted, | ||
| isItemFaded, | ||
| isItemHighlighted, | ||
| isItemFocused, | ||
| seriesId, | ||
| ], | ||
| [data, seriesId, isItemHighlighted, isItemFaded, isItemFocused, series, faded, highlighted], | ||
| ); | ||
|
|
||
| return dataWithHighlight; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,6 +95,7 @@ export default chartsApiPages; | |
| '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', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| 'x-charts/src/ScatterChart/BatchScatter.tsx', | ||
| 'x-charts-premium/src/ChartsRenderer/ChartsRenderer.tsx', | ||
| 'x-charts-premium/src/ChartsRenderer/components/PaletteOption.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 a breaking change? Someone using
PieArcPlotwill 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.
Yes but it's a legal breaking change O:)