Skip to content

[charts-pro][ChartsToolbarRangeButtons] Add initialRangeKey support#22106

Open
mustafajw07 wants to merge 5 commits intomui:masterfrom
mustafajw07:feature/22067-add-initialRangeKey-to-range-button
Open

[charts-pro][ChartsToolbarRangeButtons] Add initialRangeKey support#22106
mustafajw07 wants to merge 5 commits intomui:masterfrom
mustafajw07:feature/22067-add-initialRangeKey-to-range-button

Conversation

@mustafajw07
Copy link
Copy Markdown

@mustafajw07 mustafajw07 commented Apr 15, 2026

Changelog

@code-infra-dashboard
Copy link
Copy Markdown

code-infra-dashboard Bot commented Apr 15, 2026

Bundle size

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 🔺+951B(+0.19%) 🔺+268B(+0.18%)
@mui/x-charts-premium 🔺+951B(+0.18%) 🔺+249B(+0.16%)
@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

Deploy preview

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


Check out the code infra dashboard for more information about this PR.

optionsLookup,
),
activeRangeButtonKey: null,
activeRangeButtonKey: initialRangeKey ?? null,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The initial range key should also impact the initial zoomData. For example if initialRangeKey is set to 3 months the initial zoom should correspond to 3 months.

By order of priority the zoom shoudl be se by

  1. controlled zoom
  2. initial zoom
  3. initial range button
  4. default value

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated the pr

@alexfauquette
Copy link
Copy Markdown
Member

@mustafajw07 thank you for this pull request! It looks like your changes impact the commercially licensed code. For any changes of this nature, we require contributors to sign the MUI’s Contributor License Agreement (CLA).
I can’t find an already signed CLA that cover your changes. Please follow those steps: https://mui-org.notion.site/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 to sign it. Thanks.

@alexfauquette alexfauquette added the CLA: required Head to https://mui-org.notion.site/92ece655b1584b10b00e4de9e67eed. label Apr 16, 2026
@zannager zannager added the scope: charts Changes related to the charts. label Apr 16, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 16, 2026

Merging this PR will not alter performance

✅ 14 untouched benchmarks


Comparing mustafajw07:feature/22067-add-initialRangeKey-to-range-button (82bca79) with master (4f6dda5)1

Open in CodSpeed

Footnotes

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

@mustafajw07
Copy link
Copy Markdown
Author

@mustafajw07 thank you for this pull request! It looks like your changes impact the commercially licensed code. For any changes of this nature, we require contributors to sign the MUI’s Contributor License Agreement (CLA). I can’t find an already signed CLA that cover your changes. Please follow those steps: https://mui-org.notion.site/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 to sign it. Thanks.

I have signed the CLA.

@zannager
Copy link
Copy Markdown
Member

@mustafajw07 can you please complete the last step? Thanks.
image

@mustafajw07
Copy link
Copy Markdown
Author

@mustafajw07 can you please complete the last step? Thanks. image

Done

@zannager zannager added CLA: signed Head to https://mui-org.notion.site/92ece655b1584b10b00e4de9e67eed. and removed CLA: required Head to https://mui-org.notion.site/92ece655b1584b10b00e4de9e67eed. labels Apr 17, 2026
@zannager
Copy link
Copy Markdown
Member

@alexfauquette, CLA signed, thanks.

@JCQuintas
Copy link
Copy Markdown
Member

JCQuintas commented Apr 20, 2026

@alexfauquette I don't like that this is a two way setter. Like, the initialZoom affects the initialRangeButton and the other way around too.

Ideally we should have a single way of setting everything.

IMO we should try to calculate the selected buttons based on the current resolved zoom instead of trying to have a "priority order over multiple options"

Eg:

We calculate if each button is selected by using the selectorChartAxisZoomData, whenever the zoom changes, we calculate it again.

This way we can even get rid of activeRangeButtonKey

@alexfauquette
Copy link
Copy Markdown
Member

I don't like that this is a two way setter. Like, the initialZoom affects the initialRangeButton and the other way around too.

I suggested the second way, not the first one.

You want the initial zoom top be on the last 3 months. would you prefer to

  1. Compute the percentage that correspond to the last 3 months based on the data range
  2. Set an initialRangeButton="3-months"

IMO it's just a syntax sugar for the initialZoom

This way we can even get rid of activeRangeButtonKey

I agree it would be more flexible, but we will need to deal with the notion of precision

@JCQuintas
Copy link
Copy Markdown
Member

I don't like that this is a two way setter. Like, the initialZoom affects the initialRangeButton and the other way around too.

I suggested the second way, not the first one.

You want the initial zoom top be on the last 3 months. would you prefer to

Wouldn't it be better to change initialZoom to allow ranges then?

@JCQuintas JCQuintas added plan: Pro Impact at least one Pro user. 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 Apr 21, 2026
@JCQuintas
Copy link
Copy Markdown
Member

An option would be something like this: #22137

It would hardly ever match when users manually zoom though

@alexfauquette
Copy link
Copy Markdown
Member

Wouldn't it be better to change initialZoom to allow ranges then?

That could be a nice option 👍

@mustafajw07
Copy link
Copy Markdown
Author

Wouldn't it be better to change initialZoom to allow ranges then?

That could be a nice option 👍

Thanks for the feedback, this makes sense.

I see the concern about introducing multiple ways to control the same behavior and the current two-way relationship between initialZoom and initialRangeKey.

Based on the discussion, should i refactor the approach to make Zoom the single source of truth:

  • Remove the need for activeRangeButtonKey state
  • Derive the selected range button from the resolved zoom (instead of storing it)
  • Avoid two-way sync between range buttons and zoom
  • Treat range buttons as helpers that only update zoom

Would like to know your thoughts @JCQuintas @alexfauquette?

@JCQuintas
Copy link
Copy Markdown
Member

Based on the discussion, should i refactor the approach to make Zoom the single source of truth:

  • Remove the need for activeRangeButtonKey state
  • Derive the selected range button from the resolved zoom (instead of storing it)
  • Avoid two-way sync between range buttons and zoom
  • Treat range buttons as helpers that only update zoom

Would like to know your thoughts @JCQuintas @alexfauquette?

I think we can go into that direction. It would probably be better to divide it into multiple PRs though.

@mustafajw07
Copy link
Copy Markdown
Author

Based on the discussion, should i refactor the approach to make Zoom the single source of truth:

  • Remove the need for activeRangeButtonKey state
  • Derive the selected range button from the resolved zoom (instead of storing it)
  • Avoid two-way sync between range buttons and zoom
  • Treat range buttons as helpers that only update zoom

Would like to know your thoughts @JCQuintas @alexfauquette?

I think we can go into that direction. It would probably be better to divide it into multiple PRs though.

That direction sounds good.

Do you want this to be split into separate tickets first, or should I directly break it down into multiple PRs?

@JCQuintas
Copy link
Copy Markdown
Member

Here is the new issue for changing the initialZoom behaviour
#22158

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA: signed Head to https://mui-org.notion.site/92ece655b1584b10b00e4de9e67eed. plan: Pro Impact at least one Pro user. 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.

4 participants