[charts] Update zoom slider design #17682
Conversation
|
Thanks for adding a type label to the PR! 👍 |
bernardobelchior
added
enhancement
|
Deploy preview: https://deploy-preview-17682--material-ui-x.netlify.app/ Updated pages: |
CodSpeed Performance ReportMerging #17682 will not alter performanceComparing Summary
|
bernardobelchior
force-pushed
the
update-zoom-slider-design
branch
from
f4f3118 to
ba7b621
Compare
| const data = Array.from({ length: dataLength }).map((_, i) => ({ | ||
| x: i, | ||
| y: 50 + Math.sin(i / 5) * 25, | ||
| })); | ||
| const series2Data = Array.from({ length: dataLength }).map((_, i) => ({ | ||
| x: i, | ||
| x: i + 10, |
There was a problem hiding this comment.
If your issue is the fact that initially both series perfectly overlap, the easiest might be to modify the frequency, with something like Math.sin(i / 10)
There was a problem hiding this comment.
Yeah, that was the idea. I'll see if that looks better
| @@ -62,8 +62,6 @@ You can provide an overview and allow the manipulation of the zoomed area by set | |||
|
|
|||
| {{"demo": "ZoomSlider.js"}} | |||
|
|
|||
| Optionally, you can set the `zoom.slider.size` property to customize the zoom slider's size, that is, the height on an x-axis and the width on a y-axis. | |||
There was a problem hiding this comment.
I assume you removed this option because the size might vary according to the presence or not of the preview.
If yes, I would be in favor of keeping it, and adapting the default value according to the presence (or not) of a preview (when we introduce it).
This size does not need to be the size of the slider itself, but the size allocated to render the slider.
Basically here you would keep all you constants, and use the ZOOM_SLIDER_SIZE as a default value for zoom.slider.size. Such that users could create smaller/larger sliders and use adapt this value
There was a problem hiding this comment.
I removed this option because the size is now kind of fixed. If we increase the size, how do we scale all the components of the slider?
and use the ZOOM_SLIDER_SIZE as a default value for zoom.slider.size. Such that users could create smaller/larger sliders and use adapt this value
And center the slider in that size? Yeah, I could do that. We would need to have a minimum value, though, to avoid bleeding onto other elements of the chart. We could remove the internal concept of margin in that case.
There was a problem hiding this comment.
We could remove the internal concept of margin in that case.
For me you can keep all the rest as it is.
The component renders a fixed size slider, but the size available for the slider can be customized.
For now the only usage would be if a user create his own super small slider and then need to set zoom.slider.size = 10.
But I assume with time we will make the creation of custom slider easier and easier so it worths keeping this entry point
There was a problem hiding this comment.
Makes sense. I'll bring that code back in a follow-up where we allow the slider to be customized using a slot.
scripts/x-charts.exports.json
Outdated
| { "name": "ZOOM_SLIDER_MARGIN", "kind": "Variable" }, | ||
| { "name": "ZOOM_SLIDER_SIZE", "kind": "Variable" } |
There was a problem hiding this comment.
Do we need to export those values?
There was a problem hiding this comment.
No, it was an oversight
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
bernardobelchior
force-pushed
the
update-zoom-slider-design
branch
from
f272e5d to
47aab94
Compare
github-actions
bot
removed
the
PR: out-of-date
Part of #15383.
Update zoom slider to follow the designs:
Implementation: