Skip to content
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] Allow disabling the left and right thumbs swap #25547

Merged
merged 6 commits into from Mar 31, 2021

Conversation

michal-perlakowski
Copy link
Contributor

@michal-perlakowski michal-perlakowski commented Mar 29, 2021

Resolves #24831.

I'm not sure if the API and demo descriptions are good, so any suggestions are welcome.

Preview: https://deploy-preview-25547--material-ui.netlify.app/components/slider/#range-slider

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 29, 2021

Details of bundle changes

@material-ui/unstyled: parsed: +0.69% , gzip: +1.06%

Generated by 🚫 dangerJS against 9c0eb6f

@oliviertassinari oliviertassinari added component: avatar This is the name of the generic UI component, not the React module! component: slider This is the name of the generic UI component, not the React module! and removed component: avatar This is the name of the generic UI component, not the React module! labels Mar 29, 2021
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prop API description and demo could use an additional explainer. It isn't obvious what "swap" means. Especially when this swap might occur.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Agree, please create a new section in the documentation. Don't recycle the existing demos.
  • In the "Minimum distance shift" demo, the constraint isn't respected when reaching the edges.
  • When disableSwap is true, I believe the other thumbs shouldn't show the value on hover.

@michal-perlakowski
Copy link
Contributor Author

@oliviertassinari I fixed all issues.

  • When disableSwap is true, I believe the other thumbs shouldn't show the value on hover.

I disabled showing the value, but I wonder if I should also disable the hover effect.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to completely fix the UX on the new demo. At this point, I think that we only miss test cases to merge. The problem I went after:

  • keyboard page up => value not bound
  • keyboard right => swap of the index
  • fast mouse move => stuck with a distance between the two thumbs above the min value

@oliviertassinari oliviertassinari merged commit 7729b92 into mui:next Mar 31, 2021
@oliviertassinari
Copy link
Member

@michal-perlakowski Thanks for taking on this effort and clear the path toward a solution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slider] Allow disabling the left and right thumbs swap
4 participants