-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Slider][base][material][joy] Fix not draggable on the edge when disableSwap={true}
#35998
Conversation
Netlify deploy previewhttps://deploy-preview-35998--material-ui.netlify.app/ Bundle size report |
For more context, see #25547 |
got it, i'll fix it. according to this comment values should not be displayed on hover and that is missing in this PR #25547 (review). |
I'm kind of stuck here, According to this comment (#25547 (review)) When I tried to make the thumb draggable and hide values on hover but couldn't do it without making significant changes. I guess only one of the behavior is achievable here. |
disableSwap
disableSwap
disableSwap
@oliviertassinari Do you have any context to share on this? |
disableSwap
disableSwap={true}
I would go one step beyond: diff --git a/packages/mui-material/src/Slider/Slider.js b/packages/mui-material/src/Slider/Slider.js
index 262741a6eb..9a06641a65 100644
--- a/packages/mui-material/src/Slider/Slider.js
+++ b/packages/mui-material/src/Slider/Slider.js
@@ -765,7 +765,8 @@ const Slider = React.forwardRef(function Slider(inputProps, ref) {
})}
style={{
...style,
- pointerEvents: disableSwap && active !== index ? 'none' : undefined,
+ // So the non active thumb doesn't show its label on hover.
+ pointerEvents: active !== -1 && active !== index ? 'none' : undefined,
...thumbProps.style,
}}
> I think that the current UX is broken, how does it make sense to see the label here? Screen.Recording.2023-02-05.at.19.54.01.mov |
@@ -281,7 +282,6 @@ const SliderUnstyled = React.forwardRef(function SliderUnstyled< | |||
})} | |||
style={{ | |||
...style, | |||
pointerEvents: disableSwap && active !== index ? 'none' : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this logic not inside getThumbProps()
of useSlider? It seems that we have to fix 3 different places at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved logic to getThumbProps
but couldn't correctly type thumbProps
hence had to use @ts-ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change not done in getThumbProps
as pointed out by Olivier? We anyway provide there a style object that needs to be spreaded. Should we extend the getThumbProps
to receive additional state other then event handlers? cc @michaldudak for thoughts.
It was done in 2e8d009, but it didn't look correct with |
I now it was done, this is why I mentioned it again. We can provide an additional function in the |
What is the status of this PR? Can it be merged? Currently manually overwriting styles to get this to work.
|
put |
@mnajdova @sai6855 From the point of view of the |
@michaldudak added |
disableSwap={true}
disableSwap={true}
Looks good! |
Fixes #35971
preview: https://deploy-preview-35998--material-ui.netlify.app/material-ui/react-slider/#minimum-distance