Skip to content

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Aug 20, 2025

@sai6855 sai6855 marked this pull request as draft August 20, 2025 11:37
@sai6855 sai6855 added type: bug It doesn't behave as expected. scope: charts Changes related to the charts. labels Aug 20, 2025
@mui-bot
Copy link

mui-bot commented Aug 20, 2025

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

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%) ▼-1B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts 🔺+730B(+0.23%) 🔺+269B(+0.28%)
@mui/x-charts-pro 🔺+802B(+0.20%) 🔺+429B(+0.34%)
@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%) 🔺+1B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 82a06c3

Copy link

codspeed-hq bot commented Aug 20, 2025

CodSpeed Performance Report

Merging #19257 will not alter performance

Comparing sai6855:charts-toolbar (82a06c3) with master (92e4dc2)1

Summary

✅ 10 untouched

Footnotes

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

Comment on lines 93 to 103
[`&:has(.${chartsToolbarClasses.root})`]: {
position: 'relative',
paddingTop: '56px',
},
[`& > .${chartsToolbarClasses.root}`]: {
position: 'absolute',
top: 0,
left: '50%',
transform: 'translateX(-50%)',
zIndex: 1,
},
Copy link
Member

Choose a reason for hiding this comment

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

I would rather not use absolute positioning to fix this.

Maybe we can make use of the grid css prop to position things in the correct places, like using grid-template-areas

  /* Regular */
  grid-template-areas: 
    "toolbar"
    "legend"
    "chart";

  /* Left */
  grid-template-areas: 
    "toolbar toolbar"
    "chart legend";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I'll explore this direction

@mui-bot
Copy link

mui-bot commented Sep 2, 2025

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid ▼-4.35KB(-1.12%) 🔺+3.96KB(+3.51%)
@mui/x-data-grid-pro ▼-3.58KB(-0.76%) 🔺+5.57KB(+4.11%)
@mui/x-data-grid-premium 🔺+5.94KB(+1.02%) 🔺+10KB(+5.98%)
@mui/x-charts ▼-1.64KB(-0.52%) 🔺+3.69KB(+3.91%)
@mui/x-charts-pro 🔺+9.03KB(+2.29%) 🔺+8KB(+6.72%)
@mui/x-date-pickers ▼-4.15KB(-1.79%) 🔺+1.15KB(+1.86%)
@mui/x-date-pickers-pro ▼-5.95KB(-1.84%) 🔺+1.29KB(+1.55%)
@mui/x-tree-view ▼-785B(-1.33%) 🔺+575B(+3.20%)
@mui/x-tree-view-pro ▼-717B(-0.89%) 🔺+706B(+2.83%)

Details of bundle changes

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

github-actions bot commented Sep 2, 2025

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

Comment on lines 67 to 68
return `"toolbar toolbar"
"chart chart"`;
Copy link
Member

Choose a reason for hiding this comment

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

Would this work?

Suggested change
return `"toolbar toolbar"
"chart chart"`;
return `"toolbar"
"chart"`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 62 to 63
direction?: Direction,
position?: Position,
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 make these mandatory? Same applies to the other functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const getTemplateRows = (hideLegend: boolean, direction?: Direction) => {
if (direction === 'vertical') {
if (hideLegend) {
return '1fr';
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't 'auto' work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (position?.horizontal === 'end') {
return 'flex-end';
}
return `${width ? 'auto' : '1fr'} auto`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to handle a tricky migration issue here. When moving from flexbox to grid, we lost the automatic width handling that flexbox gives us for free.

With flexbox, child elements would automatically size correctly whether they had an explicit width (like width: 200px) or needed to expand (width: 100%). Grid doesn't do this - you have to tell it upfront how to size the columns.

My fix was to make the grid columns conditional based on whether the component has a width prop. If there's a width, use auto so it respects the explicit sizing. If no width, use 1fr so the element can expand to fill the space.

This way we keep the same flexible behavior we had with flexbox.

Copy link
Member

@JCQuintas JCQuintas Sep 15, 2025

Choose a reason for hiding this comment

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

Can't we use minmax(auto, 1fr) auto in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this, but it’s not working. The layout keeps defaulting to 1fr whenever there’s extra space available. For example, in this demo (https://deploy-preview-19257--material-ui-x.netlify.app/x/react-charts/pie/#basics
), the chart container width defaults to 1fr, but the chart itself stays fixed at 200px.

image

@sai6855 sai6855 marked this pull request as ready for review September 12, 2025 17:17
@bernardobelchior
Copy link
Member

Thanks, @sai6855, for your contribution! This PR seems to fix the bug from what I've tested.

I'm wondering if we should consider a longer term solution. We can't do breaking changes at the moment, but I'd like to reflect on what a good API for laying a chart's elements out are. Currently, I'm entertaining the idea of using gridTemplate. By default, we'd render the toolbar on top, followed by the legend and finally the chart.

Developers who want to customize the position of any of these elements can use CSS to apply a more specific gridTemplate. Since the toolbar is always rendered in the "toolbar" grid area and the same applies to the legend and the chart, a user who wants to render the legend on the right would only need to do:

gridTemplate: "toolbar ."
              "chart legend";

We could then remove the position slot props for legends because they wouldn't be necessary.

@JCQuintas @alexfauquette what do you think of this long-term proposal? If it makes sense, we could consider reshaping this PR to start moving towards that vision while keeping backwards compatibility where possible.

@JCQuintas
Copy link
Member

what do you think of this long-term proposal?

I feel that is too hidden away tbh. It would work nicely with base-ui probably eventually, but as a component we probably want to have props for that instead.

@bernardobelchior
Copy link
Member

bernardobelchior commented Sep 15, 2025

I feel that is too hidden away tbh. It would work nicely with base-ui probably eventually, but as a component we probably want to have props for that instead.

Do you have any suggestion on how that would be implemented? If we want to position the toolbar the same way the legend is positioned, how do we disambiguate when both have position.vertical === 'middle' and position.horizontal === 'end'? Which one comes first?

@JCQuintas
Copy link
Member

What do you mean by "disambiguate"? The toolbar would be on its own, so we would have to add a row to the top or bottom of the current rows. The toolbar can take the full width and if it needs to be aligned to a side it can align itself.

As far as I understand the toolbar does't have a vertical layout, so it should be simpler, as position.vertical === 'middle' wouldn't do anything on it.

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.

Seems to be working nicely. Added a suggestion to get rid of getGridTemplateAreasWithToolBar

),
},
[`&:not(:has(.${chartsToolbarClasses.root}))`]: {
gridTemplateAreas: getGridTemplateAreasWithoutToolBar(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gridTemplateAreas: getGridTemplateAreasWithoutToolBar(
gridTemplateAreas: getGridTemplateAreas(

Comment on lines 166 to 170
gridTemplateAreas: getGridTemplateAreasWithToolBar(
ownerState.hideLegend,
ownerState.legendDirection,
ownerState.legendPosition,
),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gridTemplateAreas: getGridTemplateAreasWithToolBar(
ownerState.hideLegend,
ownerState.legendDirection,
ownerState.legendPosition,
),
gridTemplateAreas: addToolbar(getGridTemplateAreas(
ownerState.hideLegend,
ownerState.legendDirection,
ownerState.legendPosition,
)),

return 'center';
};

const getGridTemplateAreasWithToolBar = (
Copy link
Member

@JCQuintas JCQuintas Sep 15, 2025

Choose a reason for hiding this comment

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

Suggested change
const getGridTemplateAreasWithToolBar = (
const addToolbar = (template: string) => {
const length = template.split('\n').at(0).split(' ').length
let toolbar = 'toolbar'
for (let i = 1; i < length; i+=1) {
toolbar += ' toolbar'
}
return `${toolbar}
${template}`
}

};

const getAlign = (direction?: Direction, position?: Position) => {
const getGridTemplateAreasWithoutToolBar = (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const getGridTemplateAreasWithoutToolBar = (
const getGridTemplateAreas = (

@bernardobelchior
Copy link
Member

What do you mean by "disambiguate"?

If the legend and the toolbar have the equal position props, in which order should they render? It's ambiguous because the answer isn't clear. We can dictate the order, but users will only know after they try it.

As far as I understand the toolbar does't have a vertical layout, so it should be simpler, as position.vertical === 'middle' wouldn't do anything on it.

Yeah, it doesn't at the moment, but the problem is still valid if we can only position the toolbar on top and bottom.


My concern is that the current API isn't enough to fully customize the positioning of both the toolbar and the legend. This PR fixes the bug it references, but I think the API can still be improved.

@sai6855
Copy link
Contributor Author

sai6855 commented Sep 16, 2025

@JCQuintas Removed both getGridTemplatesWithToolBar and hetGridTemplatesWithoutToolBar with getGridTemplates here 82a06c3

addToolBar function turned out to be simply appending toolbar to template, so i used that. 82a06c3#diff-d76ba0f0d9f1045e0a6ffa0a7ddc7e21b98dd2739008de6bed96839f9a1ce496R58-R60

@sai6855 sai6855 requested a review from JCQuintas September 16, 2025 10:10
@sai6855 sai6855 merged commit 2a750fb into mui:master Sep 16, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: charts Changes related to the charts. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[charts] Legend position affects the toolbar's position

4 participants