-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
[Slider][material-next] Slider restructure and style improvements #37644
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][material-next] Slider restructure and style improvements #37644
Conversation
Netlify deploy previewhttps://deploy-preview-37644--material-ui.netlify.app/ @mui/material-next: parsed: +57.44% , gzip: +59.19% Bundle size report |
mnajdova
left a comment
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.
Overall it looks great! Left some comments for improvements.
| import { unstable_useEnhancedEffect as useEnhancedEffect } from '@mui/utils'; | ||
|
|
||
| function getSliderElementsOverlap(elements: HTMLElement[], axis: Axis, margin: number) { | ||
| if (elements.length === 2) { |
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.
if we accept an array, let's make the hook considering all elements in the array, it shouldn't be much change.
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.
I agree! But rather than considering all elements, I modified it to only accept two at most. I think this makes more sense as the slider thumb and value label elements should be two at most as well.
|
Hey @mnajdova, thanks for the review! I addressed your comments and also I added the v6 components migration guide, that way we can begin documenting breaking changes alongside PRs, with this PR introducing the first. |
| @@ -0,0 +1,38 @@ | |||
| # Breaking changes in v6: core components | |||
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.
Instead of page, can we please add readme file in the @mui/material-next package? Maybe with name migration.md next to readme. We can add info in the readme, that they should follow the migraiton.md guidelines.
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.
The changes look good to me, I left one suggestion for the migration location, as we anyway don't want to add a page on the docs just yet. @DavidCnoops can you review the design-related changes?
DavidCnoops
left a comment
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.
Overall this looks really good to me! It's a great solution to a tricky UI problem.
I have one very tiny nitpick: when valueLabelDisplay is set to auto and the 2 thumbs/values are about to overlap, the underlying value very briefly pops up. Also, when sliding the upper thumb over the lower one (i.e. when they completely overlap) its value seems to briefly disappear. It's -extremely- subtle so definitely not a big or blocking issue, but a 100% perfect solution would be to keep the lower value constantly hidden and the upper value constantly visible.
slider.webm
960fdba to
91b6a95
Compare
91b6a95 to
b166d5f
Compare
|
Hey @DavidCnoops! thanks for reviewing 😊 What you reported was indeed a bug:
Thanks for catching the issues 🚀 |
mnajdova
left a comment
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.
We have a demo of a Slider with three thumbs - http://localhost:3000/material-ui/react-slider/#removed-track, also as far as I remember we updated the Base UI Slider and useSlider to support more than two thumbs. I would propose making this scenario works too.
packages/mui-material-next/src/Slider/useSliderElementsOverlap.ts
Outdated
Show resolved
Hide resolved
packages/mui-material-next/src/Slider/useSliderElementsOverlap.ts
Outdated
Show resolved
Hide resolved
|
Hey @mnajdova thanks for the feedback, I included your changes in the last commit:
|
f7f27c7 to
6ad5568
Compare
mnajdova
left a comment
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.
Looks much better 👍 I would recommend for PRs that include fixes like this to share a codesandbox for easily verifying the fix. It would help a lot with the reviews :)
Material Next Slider issue: #37527
Summary
This PR implements improvements that required a restructuring of the Slider Thumb and Value Label structure. The original PR introducing
material-next's Slider is #37520The improvements are:
Thumb restructure
Address this comment. Restructure the Thumb and value label to a simpler structure, like the one in Joy's Slider. This removes the need for a separate ValueLabel component and types file, so those were removed.
Thumb and value label overlap
Implement the overlap border specification from Material You.
To do so I introduced the
useSliderElementsOverlaphook that handles the logic of detecting overlaps for thumbs and value labels.Screen.Recording.2023-06-19.at.14.22.47.mov