[charts-pro] Add size for zoom slider #17736
Conversation
|
Thanks for adding a type label to the PR! 👍 |
bernardobelchior
added
enhancement
|
Deploy preview: https://deploy-preview-17736--material-ui-x.netlify.app/ Updated pages: |
bernardobelchior
force-pushed
the
zoom-slider-size
branch
from
1568a65 to
260ce4b
Compare
CodSpeed Performance ReportMerging #17736 will not alter performanceComparing Summary
|
| You can set the `zoom.slider.size` property to customize the size reserved for the zoom slider's. | ||
| Since the zoom slider has a fixed size of 20 px, updating `zoom.slider.size` effectively changes the margin around the slider. | ||
|
|
||
| The size is the height on an x-axis and the width on a y-axis. |
There was a problem hiding this comment.
Not sure if it's worth documenting this. I still feel a bit uneasy that we're calling it size, but it only affects the margin.
Should we maybe call it margin instead?
Another drawback of size is that if a user sets a size small than 20, we'll see the slider bleeding onto other elements.
There was a problem hiding this comment.
I got confused by this, but now I noticed the slider is an svg element.
I was assuming it to be HTML 🤯
What was the rationale for SVG vs HTML? 🤔
I assumed this interactive part would be easier to handle by having a HTML, since when the preview is added, the scales/drawingarea will have to be recalculated anyways.
This would have the benefit of not having to calculate sizes, but I haven't thought about it too deeply.
There was a problem hiding this comment.
I made it SVG because we can have stacked axis on one side and the slider should be next to the axis it represents, so to use HTML meant we'd need to use a foreignObject, which I wanted to avoid.
Even if we use HTML, we'd need to calculate size because if we have an axis below a zoom slider, we'd need to be observing the sliders size to update the axes' offsets so they wouldn't overlap.
Are you seeing any approach with HTML that makes it simpler to handle this case?
There was a problem hiding this comment.
I made it SVG because we can have stacked axis on one side and the slider should be next to the axis it represents, so to use HTML meant we'd need to use a
foreignObject, which I wanted to avoid.
I hadn't thought of it, though I suspect this to be such and edge-case that it might not be worth optimising for it.
Even if we use HTML, we'd need to calculate size because if we have an axis below a zoom slider, we'd need to be observing the sliders size to update the axes' offsets so they wouldn't overlap.
WDYM?
If the HTML is in the HTML part of the charts it would behave just like the legend does right now, which is automatic layout.
There was a problem hiding this comment.
If the HTML is in the HTML part of the charts it would behave just like the legend does right now, which is automatic layout.
If we want to have this edge case working properly:
Then we need to offset the second axis by the size of the slider so that they don't overlap.
How could we fix this with HTML?
There was a problem hiding this comment.
I just wouldn't solve it. Like, this is already an edge case, it is visually complex for the users to understand too.
It is probably easier both on our and the user side to couple visually similar elements together, and let the users figure out which controls which by using it. We can help by keeping them in order though 😆
If a dev really wants to have them in different order they can play with the axis size and absolute positions. But again, that would be a super edge-case
There was a problem hiding this comment.
this is already an edge case
Yeah, I agree with this.
If a dev really wants to have them in different order they can play with the axis size and absolute positions.
Yeah, I can agree with this.
Is it worth converting everything to HTML now, though? I can do it if needed.
@alexfauquette what's your take on this?
Follow-up to this comment.