[Slider] Fix positioning of tooltips on vertical slider #32919
Conversation
There was a problem hiding this comment.
The demos look better now, but there's a regression in https://deploy-preview-32919--material-ui.netlify.app/material-ui/customization/theme-components/#overrides-based-on-props
michaldudak
changed the title
michaldudak
added
component: slider
There was a problem hiding this comment.
Thoughts for follow-ups:
- It could be great to add a visual regression test with
valueLabelDisplay="on", to avoid regressions. - The transition's origin is wrong:
broken.mp4
- We could have in the Slider docs a section specifically for the
valueLabelDisplayprop. We currently mix the values in the demos, it's a bit confusing.
| @@ -20,6 +20,7 @@ export default function VerticalSlider() { | |||
| orientation="vertical" | |||
| defaultValue={30} | |||
| aria-label="Temperature" | |||
| valueLabelDisplay="auto" | |||
There was a problem hiding this comment.
Are we sure about this change? Why not keep the default value?
There was a problem hiding this comment.
With it, developers can see how the value tooltips look like on vertical sliders. I'd keep them.
There was a problem hiding this comment.
It feels like we made this change only to test & develop the fix. I wonder if a general rule should be to either make it the default behavior or remove the prop in the demos (instead, replacing it with a dedicated demo) 😁.
This thing will be fixed by adding |
|
@abhinav-22-tech Could you create a PR with this fix? |
|
I have also missed this: 4. wrong vertical alignment #33009 (comment) |
Closes: #31103
Preview: https://deploy-preview-32919--material-ui.netlify.app/material-ui/react-slider/#vertical-sliders