Skip to content

Conversation

@alexfauquette
Copy link
Member

No description provided.

@mui-bot
Copy link

mui-bot commented Dec 9, 2025

Deploy preview: https://deploy-preview-20604--material-ui-x.netlify.app/

Updated pages:

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 🔺+1.87KB(+0.41%) 🔺+265B(+0.19%)
@mui/x-charts-premium 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against f69f84f

@zannager zannager added the scope: charts Changes related to the charts. label Dec 12, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 12, 2025

CodSpeed Performance Report

Merging #20604 will not alter performance

Comparing alexfauquette:sankey-update (f69f84f) with master (dcbbc64)1

Summary

✅ 14 untouched

Footnotes

  1. No successful run was found on master (f76cfa0) during the generation of this report, so dcbbc64 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@alexfauquette
Copy link
Member Author

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)

@JCQuintas
Copy link
Member

Did my best to simplify but ended up to the conclusion that we can not display all those labels on mobile

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 😆

Copy link
Member Author

@alexfauquette alexfauquette left a 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

Comment on lines 123 to 146
<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}
>
Copy link
Member Author

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?

Comment on lines 15 to 19
import { sankeySeriesConfig } from '@mui/x-charts-pro/SankeyChart/seriesConfig';
import {
SANKEY_CHART_PLUGINS,
SankeyChartPluginSignatures,
} from '@mui/x-charts-pro/SankeyChart/SankeyChart.plugins';
Copy link
Member Author

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

Copy link
Member

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];
Copy link
Member Author

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

Copy link
Member

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

Comment on lines +41 to 42
const SankeyPlotRoot = styled('g')({});

Copy link
Member Author

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) {
Copy link
Member Author

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

Suggested change
export function useSankeyNodeHighlightState(nodeId: SankeyNodeId) {
export function useSankeyNodeHighlightState(nodeId: SankeyNodeId): 'highlighted' | 'faded' | 'none' {

Copy link
Member

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?

Copy link
Member Author

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 }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

@alexfauquette alexfauquette changed the title [WIP][charts] Try some label improvement on sankey [charts-pro] Support composition for Sankey Dec 16, 2025
Copy link
Member

@JCQuintas JCQuintas left a 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

Comment on lines 15 to 19
import { sankeySeriesConfig } from '@mui/x-charts-pro/SankeyChart/seriesConfig';
import {
SANKEY_CHART_PLUGINS,
SankeyChartPluginSignatures,
} from '@mui/x-charts-pro/SankeyChart/SankeyChart.plugins';
Copy link
Member

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) {
Copy link
Member

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];
Copy link
Member

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 17, 2025
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 22, 2025
@alexfauquette alexfauquette added docs Improvements or additions to the documentation. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: charts Changes related to the charts. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants