-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts-pro] Support composition for Sankey #20604
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
base: master
Are you sure you want to change the base?
Conversation
|
Deploy preview: https://deploy-preview-20604--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Performance ReportMerging #20604 will not alter performanceComparing Summary
Footnotes |
|
Did my best to simplify but ended up to the conclusion that we can not display all those labels on mobile @JCQuintas About the approach to allow users for creating this same node labels. I could introduce a slot 😈 But I'm considering exporting a SankeyNodePlot, SankeyLinkPlot, SankeyNodeLablePlot, SankeyLinkLablePlot to do the demo with composition (reuse SankeyNodePlot, SankeyLinkPlot and do custom SankeyNodeLablePlot, SankeyLinkLablePlot) |
Yes, an option for mobile should be to make the sankey vertical, or provide the labels outside the context of the chart. I didn't want to implement composition for the sankey before the headless migration, so we don't have yet another slot to move over 😆 |
alexfauquette
left a comment
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.
@JCQuintas I tried to make something that would adapt well to a headless version
| <ChartDataProviderPro<'sankey', SankeyChartPluginSignatures> | ||
| series={[ | ||
| { | ||
| type: 'sankey' as const, | ||
| data, | ||
| valueFormatter, | ||
| nodeOptions: { | ||
| sort: 'fixed', | ||
| padding: 20, | ||
| width: 9, | ||
| showLabels: isDesktop, | ||
| }, | ||
| linkOptions: { | ||
| color: 'target', | ||
| opacity: 0.6, | ||
| curveCorrection: 0, | ||
| showValues: !isDesktop, | ||
| }, | ||
| }, | ||
| ]} | ||
| margin={{ top: 20 }} | ||
| seriesConfig={seriesConfig} | ||
| plugins={SANKEY_CHART_PLUGINS} | ||
| > |
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.
What about exporting a SankeyDataProvider that simplifies that?
| import { sankeySeriesConfig } from '@mui/x-charts-pro/SankeyChart/seriesConfig'; | ||
| import { | ||
| SANKEY_CHART_PLUGINS, | ||
| SankeyChartPluginSignatures, | ||
| } from '@mui/x-charts-pro/SankeyChart/SankeyChart.plugins'; |
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.
Either we create a SankeyDataprovider or we export those from the @mui/x-charts-pro/SankeyChart
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.
Should be ok to export the data provider
| const { link } = props; | ||
| const theme = useTheme(); | ||
| const seriesContext = useSankeySeriesContext(); | ||
| const series = useSankeySeries()[0]; |
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.
Maybe we should simplify a bit further by directly returning the first series. The fact it's an array is just due to some internal consistency issues. From the user perspective their is always only one series
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.
The component is Unstable_SankeyChart but the hooks are not. We could technically do it, but might be annoying. We could wait for v9
| const SankeyPlotRoot = styled('g')({}); | ||
|
|
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 moved those to the SankeyNodePlot and SankeyLinkPlot. It decreases default style specificity.
I kept the styled to have the theme stye override. Coudl be removed if needed
| } from './plugins/useSankeyHighlight.selectors'; | ||
| import type { SankeyLayoutLink, SankeyNodeId } from './sankey.types'; | ||
|
|
||
| export function useSankeyNodeHighlightState(nodeId: SankeyNodeId) { |
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.
Proposal for a new highlight hook API that express the fact that highlighted items can't be faded
| export function useSankeyNodeHighlightState(nodeId: SankeyNodeId) { | |
| export function useSankeyNodeHighlightState(nodeId: SankeyNodeId): 'highlighted' | 'faded' | 'none' { |
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.
Can we turn this into a general/global hook/rule?
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.
You mean for other series?
I think yes. For now hooks do to following which is the same but with more varaibles
const isHighlighted = store.use(isHighlightedSelector)
const isFaded = store.use(isFadedSelector)
return { isHighlighted, isFaded: !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.
yeah
JCQuintas
left a comment
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.
Should work, it seems like it is a bit cumbersome to create the custom label plot, but I don't think we should provide more higher-level-apis (something like useNodeLabelPosition) at this point
| import { sankeySeriesConfig } from '@mui/x-charts-pro/SankeyChart/seriesConfig'; | ||
| import { | ||
| SANKEY_CHART_PLUGINS, | ||
| SankeyChartPluginSignatures, | ||
| } from '@mui/x-charts-pro/SankeyChart/SankeyChart.plugins'; |
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.
Should be ok to export the data provider
| } from './plugins/useSankeyHighlight.selectors'; | ||
| import type { SankeyLayoutLink, SankeyNodeId } from './sankey.types'; | ||
|
|
||
| export function useSankeyNodeHighlightState(nodeId: SankeyNodeId) { |
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.
Can we turn this into a general/global hook/rule?
| const { link } = props; | ||
| const theme = useTheme(); | ||
| const seriesContext = useSankeySeriesContext(); | ||
| const series = useSankeySeries()[0]; |
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.
The component is Unstable_SankeyChart but the hooks are not. We could technically do it, but might be annoying. We could wait for v9
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
No description provided.