[charts-pro] Add range buttons to toolbar#21964
Conversation
JCQuintas
added
type: new feature
JCQuintas
changed the title
|
Deploy preview: https://deploy-preview-21964--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
|
This touches generic slots, so a lot of noise from scripts too. I'll add them once we agree on api. |
alexfauquette
left a comment
alexfauquette
left a comment
There was a problem hiding this comment.
The API look nice. But the real value would be for candle stick so with ordinal scale.
We can either go forward wit this PR (I would give a second look at the internal logic) and we add ordinal scale management latter. Or we add it in this PR. As you prefer
alexfauquette
left a comment
alexfauquette
left a comment
There was a problem hiding this comment.
I'm wonderineg if we should handle non-date. It feels a bit weird to handle that edge case.
About performances. You're looping multiple time on data arrays. to transform them into timestamp, and to find bounds. Might be better to do a binary search and get timestamp directly during the search (this can be a follow up)
| params.zoomInteractionConfig, | ||
| optionsLookup, | ||
| ), | ||
| activeRangeButtonKey: null, |
There was a problem hiding this comment.
For a follow up: a initialRangeKey props could be nice
There was a problem hiding this comment.
Same usecase as initialZoom props
For example, you open the page. By default it's zoomed to the last 3 months, and you see in the toolbar that "3 months" is selected.
Without initialRangeKey users can set an initial zoom, but have no way to let the end-user know which one it is
There was a problem hiding this comment.
Ah, I think we can eventually reverse-match
There was a problem hiding this comment.
we can eventually reverse-match
I though about it, but the tradeoff between the complexity to handle all the edge cases versus the value it bring is not that good
| expect(result.end).to.be.closeTo((2 / 3) * 100, 0.1); | ||
| }); | ||
|
|
||
| it('should fall back to continuous logic for non-date-like data', () => { |
There was a problem hiding this comment.
SHould this at least raise a warning?
| it('should work with date strings', () => { | ||
| const result = rangeButtonValueToZoom([new Date(2024, 3, 1), new Date(2024, 6, 1)], { | ||
| scaleType: 'band', | ||
| data: ['2024-01-01', '2024-04-01', '2024-07-01', '2024-10-01'], | ||
| domain: { min: 0, max: 3 }, |
There was a problem hiding this comment.
Could you add a test case with a missing date?
alexfauquette
left a comment
alexfauquette
left a comment
There was a problem hiding this comment.
🎉
Thanks for not committing the generated files, it made the PR much easier :)
| return ( | ||
| <LineChartPro | ||
| dataset={dataset} | ||
| xAxis={[{ scaleType: 'time', dataKey: 'date', zoom: true, tickNumber: 5 }]} |
There was a problem hiding this comment.
You also need a y-axis to modify it's width or format the values
| <span> | ||
| <ChartsToolbarZoomInTrigger render={<ToolbarButton size="small" />}> | ||
| <ZoomInIcon fontSize="small" {...slotProps.zoomInIcon} /> | ||
| </ChartsToolbarZoomInTrigger> | ||
| </span> |
There was a problem hiding this comment.
Why do you need those span?
There was a problem hiding this comment.
when the button is disabled the tooltip warns in the console that it can't work on a disabled element or smthing like that.
| // When no button has been clicked and zoom is at full range, the null-value button is active. | ||
| const isActive = | ||
| activeRangeButtonKey === label || | ||
| (activeRangeButtonKey === null && value === null && canZoomOut); |
There was a problem hiding this comment.
This line looks weird, because the selector name is not correct.
canZoomOut is set to true when "the chart can not zoom out"
But that's outside of this PR context
Bundle size
Deploy previewCheck out the code infra dashboard for more information about this PR. |
3 participants
Summary
ToggleButtonGroupwith visual active state that clears when the user manually zooms/pans. The reset (null) button appears selected on initial render when zoom is at full range.{ unit: 'month', step: 3 }), absolute date ranges ([Date, Date]), custom functions with axis context ({ scaleType, data, domain, zoomed }), andnull(reset).baseToggleButtonandbaseToggleButtonGroupslots.ChartsToolbarRangeButtonTriggercomponent for custom toolbar composition.rangeButtonsandrangeButtonsAxisIdprops toChartsToolbarPro.RangeButtonFunctionParamstype export for typing custom function values.RangeButtonGroupstyled component (MuiChartsToolbarRangeButtons) for toolbar button styling.gdpPerCapitaEvolutiondataset and include an ordinal axis example with monthly dates on a band axis.