[charts-pro] Add tests and classes to zoom slider #18660
Conversation
|
Deploy preview: https://deploy-preview-18660--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 🔺+2.21KB(+0.02%) - Total Gzip Change: 🔺+697B(+0.02%) Show details for 100 more bundles (22 more not shown)@mui/x-charts-pro/ChartZoomSlider parsed: 🔺+482B(+0.71%) gzip: 🔺+129B(+0.54%) |
CodSpeed Performance ReportMerging #18660 will degrade performances by 20.79%Comparing Summary
Benchmarks breakdown
|
|
|
||
| export const chartAxisZoomSliderTrackClasses: ChartAxisZoomSliderTrackClasses = | ||
| generateUtilityClasses('MuiChartAxisZoomSliderTrack', [ | ||
| 'root', |
| @@ -272,6 +274,7 @@ export function ChartAxisZoomSliderActiveTrack({ | |||
| height={previewHeight} | |||
| onPointerEnter={onPointerEnter} | |||
| onPointerLeave={onPointerLeave} | |||
| className={classes.active} | |||
There was a problem hiding this comment.
Not sure why this is showing up as Mui-active when the background is MuiChartAxisZoomSliderTrack-background 🤔
Any idea?
Screen.Recording.2025-07-02.at.15.14.28.mov
There was a problem hiding this comment.
🤔 "active" seems to be part of "global state classes" so it uses a global value instead...
I overrode it with the underlying implementation
| ]); | ||
|
|
||
| export function getAxisZoomSliderTrackUtilityClass(slot: string) { | ||
| return `${ClassNameGenerator.generate('MuiChartAxisZoomSliderTrack')}-${slot}`; |
There was a problem hiding this comment.
Can we add a code comment explaining why we did this instead of going the normal way?
Another approach would be to rename background and active to backgroundTrack and activeTrack.
There was a problem hiding this comment.
I'll add a comment instead
There was a problem hiding this comment.
unless @alexfauquette has any reason not to. In my view this is something that has a special context in the material codebase, but might not be applicable to ours
| await act(async () => new Promise((r) => requestAnimationFrame(r))); | ||
|
|
||
| expect(onZoomChange.callCount).to.equal(1); |
There was a problem hiding this comment.
Would it make sense to do this instead?
| await act(async () => new Promise((r) => requestAnimationFrame(r))); | |
| expect(onZoomChange.callCount).to.equal(1); | |
| await waitFor(() => expect(onZoomChange.callCount).to.equal(1)) |
There was a problem hiding this comment.
Would it make sense to do this instead?
We could, but errors are worse to read
| coords: { x: 50, y: 0 }, | ||
| }, | ||
| { | ||
| target: sliderTrack, | ||
| coords: { x: 20, y: 0 }, |
There was a problem hiding this comment.
If I move the mouse 30px to the left, shouldn't we check if 'E' is visible rather than 'B'?
There was a problem hiding this comment.
doesn't the slider functions the opposite way?
[B, C]
<- Pan
Movement ->
[C, D]
//
[B, C]
<- Slide
<- Movement
[A, B]There was a problem hiding this comment.
If I move my mouse 30px to the left, the chart contents are translated in the same direction:
Screen.Recording.2025-07-02.at.15.45.17.mov
Or am I missing something?
There was a problem hiding this comment.
This test is for the slider 😆
I've noticed while trying to create tests for the slider that there were no classes on the tracks. I've added them and added the tests as well.