[charts] Base the highlight items on the item identifiers#21161
[charts] Base the highlight items on the item identifiers#21161alexfauquette wants to merge 84 commits intomui:masterfrom
Conversation
|
Deploy preview: https://deploy-preview-21161--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This reverts commit d4214ff.
|
I tried changes to For some reason the selector input types were being expanded when building the community package, and since the community doesn't have sankey/heatmap it would keep these out. The solution was to simplify the selectors types by extracting the actual selector from the function types. |
| Item extends SeriesItemIdentifier<SeriesType> | SeriesItemIdentifierWithType<SeriesType>, | ||
| >( | ||
| identifier: Item, | ||
| typeOfIdentifier?: 'seriesItem', |
There was a problem hiding this comment.
Why is typeOfIdentifier needed in this function? This seems like its trying to solve a type issue with a prop. 😆
There was a problem hiding this comment.
Tbh there seems to be too many types of identifiers.
We should probably look into having them being the same.
The idea behind the VisibilityIdentifier is that is could be any Identifier-Like object.
Does the highlight identifier need to be different from the regular identifiers?
We could then have Identifier, IdentifierWithType, IdentifierLike
There was a problem hiding this comment.
It's effectively a prop that solves a type issue.
I think there is one aspect which is true: the IdentifierLike are a subset of the Identifier.
But I don't see why the Highlight and Visibility should be the same.
Tbh there seems to be too many types of identifiers.
Just one per features (tooltip, highlight, visibility)
The idea behind the VisibilityIdentifier is that is could be any Identifier-Like object.
Not exactly. It make no sens to provide a dataIndex to a visibility identifier of a bar chart.
But it make sense for the highlight.
// For the Bar identifiers, we should get
dataIndex: number, // identifier & tooltip
dataIndex?: number, // highlight
dataIndex: never, // visibility
Previously, highlight was based on
seriesId+dataIndexexcept for the sankey that got a dedicated highlight pluginPreviously
In dedicated PR we moved to get
So from those PR the highlight scope depends on the series type.
In this PR
seriesConfigHighlightItemDatais replaced by theSeriesItemIdentifier<SeriesType>HighlightItemDatabyHighlightItemIdentifier<SeriesType>. For now it's a hack that make optional the dataIndex. But can became an custom identifier per series. The underlying idea is that the controlled highlight should be able to target an item (with thanks to the dataIndex) or an entire series (by omiting the dataIndex)Remaining in follow-up
isHighlighted/isFadedit could be'highlighted' | 'faded' 'none'to express the fact that highlighted implies not faded. (to do in a follow up)