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] Replace reversed with rtl support on horizontal sliders #12972

Merged
merged 2 commits into from
Sep 27, 2018

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Sep 23, 2018

Breaking change

  • Remove the reverse property from the Slider component.

Fix: Slider interactions and thumb positions in rtl themes

This was discussed internally.

Reasons for the removal:

  • The previous implementation was completely broken for right-to-left themes
  • We currently don't have any precedent that we support local reversal of a component that is not done because of right-to-left language support.
  • Supporting the reverse prop would result in 8 different combinations of the Slider just for direction and orientation. This is very error prone.
  • It follows WAI-ARIA slider authoring practices
  • It follows the material design specs. The vertical slider is kept regardless.
  • The original PR [Slider] Add support for vertical/reversible sliders #4571 did not provide real world examples or a concrete use case

Pinging @lwansbrough in case he wishes to make a more compelling case for reverse sliders.

Follow up after merge:

  • Minimum value for vertical slider should be the bottom

This follows WAI-ARIA slider authoring practices as well as the
material design specs. The vertical slider is kept regardless.
@eps1lon eps1lon force-pushed the fix/slider-rtl-theme branch from 6ad29a2 to b47ba62 Compare September 23, 2018 11:54
@oliviertassinari oliviertassinari added the component: slider This is the name of the generic UI component, not the React module! label Sep 23, 2018
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.

Great!

@@ -37,7 +37,10 @@ Sliders reflect the current state of the settings they control.

## Reverse slider
Copy link
Member

Choose a reason for hiding this comment

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

Why not remove this section all together?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about people looking for the missing prop but the changelog entry and this PR is probably sufficient.

@oliviertassinari
Copy link
Member

The reverse property was introduced in #11040.

@sakulstra
Copy link
Contributor

Just a side note here: reverse or vertical reverse was one of the main obstacles when trying to port this to native input range components. If this pr gets merged it might make sense to reconsider a native implementation

@oliviertassinari oliviertassinari merged commit eb03795 into mui:master Sep 27, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 27, 2018

Just a side note here: reverse or vertical reverse was one of the main obstacles when trying to port this to native input range components. If this pr gets merged it might make sense to reconsider a native implementation

@sakulstra What's the advantage of the native implementation? From what I understand, we get the accessibility and form submission for free. I would love to explore this path. Especially if it means simplifying the internal implementation.

@eps1lon eps1lon deleted the fix/slider-rtl-theme branch September 27, 2018 06:54
@sakulstra
Copy link
Contributor

sakulstra commented Sep 27, 2018

@oliviertassinari what I thought the advantages would be:

  • better performance
  • native multi touch support
  • native support for step
  • i expected it to be more lightweight
  • native onchange events

The disadvantage is, that browser implementations and feature support widely differ even in modern browsers - so getting things right is unnecessarily hard:
https://css-tricks.com/sliding-nightmare-understanding-range-input/

marcelpanse pushed a commit to marcelpanse/material-ui that referenced this pull request Oct 2, 2018
…#12972)

* [Slider] Replace reversed with rtl support on horizontal sliders

This follows WAI-ARIA slider authoring practices as well as the
material design specs. The vertical slider is kept regardless.

* [docs] Remove obsolete reverse slider section
@w3nda
Copy link

w3nda commented Oct 7, 2018

current rtl behavior
https://i.gyazo.com/9258b17c7bc7aa1764c939f4d9382d7a.mp4

@sakulstra any Idea?

@oliviertassinari
Copy link
Member

@wenduzer Did you follow the 3 steps of our RTL guide?

@w3nda
Copy link

w3nda commented Oct 7, 2018

@oliviertassinari yes

@eps1lon
Copy link
Member Author

eps1lon commented Oct 7, 2018

@wenduzer Could you open a separate issue and follow the issue template? This looks like the behavior before this PR got merged.

@w3nda
Copy link

w3nda commented Oct 7, 2018

maybe npm repo isn't updated?

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

Successfully merging this pull request may close these issues.

6 participants