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] Make interaction easier, fix thumb overflow #11889

Merged
merged 3 commits into from
Jul 6, 2018

Conversation

ValentinH
Copy link
Contributor

@ValentinH ValentinH commented Jun 17, 2018

Wider the clickable zone and make the whole bar receive the initial move.

This is especially useful on mobile as today's version is hard to use on touch-screen (12 px is too small). I've made the clickable zone 40px wide. Angular Material has it at 48px FYI.

Before:
not-easy-slider

After:
easy-slide

@oliviertassinari oliviertassinari added component: slider This is the name of the generic UI component, not the React module! package: lab Specific to @mui/lab labels Jun 17, 2018
@ValentinH
Copy link
Contributor Author

Hey @oliviertassinari, any update on this?

@mbrookes
Copy link
Member

@ValentinH I haven't looked into it beyond the Argos-CI screenshots, but it looks like this is changing the positioning of the slider.

@ValentinH
Copy link
Contributor Author

Indeed, I've made the touchable zone bigger so there is a bit more space around it.

Is it a no-go or is it fine to change its positioning for good reasons? If not, I could remove some margin so the position remains the same but the touchable area is bigger.

I'm using the slider quite often on mobile and it's really hard to interact with...

@mbrookes
Copy link
Member

It isn’t a major issue - we can make breaking changes in the lab with impunity. I had just assumed that the touch area could be increased without affecting the slider positioning.

@ValentinH
Copy link
Contributor Author

We need to decide what we want. I chose to keep an extra space so we can have a large touch area and still a bit of margin between elements (for the 2 vertical sliders of the demo for example).

However, I'm not sure what is the best. Let me know what you think. 🙂

@ValentinH
Copy link
Contributor Author

Ping! 🙂

@mbrookes
Copy link
Member

@ValentinH I think if we can maintain the current size while making the interaction area bigger, that might be the best tradeoff.

There's something up with the interaction though - if you click and move the pointer away, the slider jumps to the clicked position. Previously, the slider didn't move.

Either way, looking at the spec, I'm wondering if it should slide the the clicked position as soon as it's clicked?

Also, it's possible to get the pointer "attached" to the thumb, even after the button is released by clicking on the thumb and and moving the pointer away.

@ValentinH
Copy link
Contributor Author

Ok for the size, I'll update.

I added the ability to start dragging the thumb directly by clicking anywhere on the slider. It's much easier to interact with it like this. Most sliders work like this.

I didn't notice the bug you describe in your last sentence, I'll investigate :)

@ValentinH
Copy link
Contributor Author

I have updated to size to be similar to before. However, when 2 sliders are displayed side by side, there is a bigger spacing because I replaced margin by padding and padding doesn't merge.
However, I continue to think we should make the touch area bigger so it's easier to use on mobile. With this change, it's better, but it could be even simpler with a bigger touchable zone.

Angular Material Slider touchable is much bigger for instance (48px tall):
image

I have also added a e.preventDefault() for mousedown and touchstart to prevent to bug you were mentioning.

ValentinH added 2 commits July 4, 2018 21:44
Wider the clickable zone and make the whole bar receive the initial move
@mbrookes mbrookes force-pushed the make-slider-interaction-easier branch from 17e8833 to b163c5c Compare July 4, 2018 21:08
@mbrookes
Copy link
Member

mbrookes commented Jul 4, 2018

@ValentinH I've iterated on your PR to solve a related issue, wherein the thumb would overflow the Slider bounding box. I'd appreciate your feedback!

@epodivilov I noticed some of the tests had fallen out of sync with the implementation (we don't currently run the lab tests with CI so it got missed). I've updated most of them. Would you mind taking a look and sanity checking them? Also, the onDragEnd test is failing, but I couldn't immediately see why. Any idea?

@ValentinH
Copy link
Contributor Author

@mbrookes looks perfect to me! 👍

@mbrookes mbrookes force-pushed the make-slider-interaction-easier branch from b163c5c to b0389c0 Compare July 6, 2018 22:00
@mbrookes mbrookes changed the title Slider: make interaction easier [Slider] Make interaction easier, fix thumb overflow Jul 6, 2018
@mbrookes mbrookes merged commit d62f059 into mui:master Jul 6, 2018
@mbrookes
Copy link
Member

mbrookes commented Jul 6, 2018

@ValentinH Thanks for working on this. 👍

@epodivilov If you get a chance to look at that test at some point that's be great!

@epodivilov
Copy link
Contributor

epodivilov commented Jul 7, 2018

@mbrookes I apologize for the long wait =)

I looked at the tests. Everything looks great. I did not have any problems with testing onDragEnd. Did you solve the problem or did it self-destruct?

@mbrookes
Copy link
Member

mbrookes commented Jul 7, 2018

@epodivilov that's weird, now I'm getting 'should have called the handleDragStart cb', but I didn't change anything to affect those tests 😕

Just to confirm, did you cd packages/material-ui-lab before running the tests?

@epodivilov
Copy link
Contributor

@mbrookes Yes, after cd packages/material-ui-lab I also have same error. Ok, I will try to fix this problem.

@mbrookes
Copy link
Member

mbrookes commented Jul 7, 2018

@epodivilov Wonderful, thank you!

acroyear pushed a commit to acroyear/material-ui that referenced this pull request Jul 14, 2018
@ValentinH ValentinH deleted the make-slider-interaction-easier branch July 17, 2018 08:03
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Oct 3, 2018
Document how to apply mui#11889 which was removed from default
implementation in mui#12967.
eps1lon added a commit that referenced this pull request Oct 4, 2018
* [Slider] Lowest value for vertical should be at the bottom

* [Slider] Fix lagging transition on vertical slider

* [Slider] Fix thumb position on horizontal sliders

* [docs] Document how to improve actionable area

Document how to apply #11889 which was removed from default
implementation in #12967.
@eps1lon eps1lon mentioned this pull request May 2, 2019
1 task
@oliviertassinari oliviertassinari added new feature New feature or request and removed package: lab Specific to @mui/lab labels Jan 9, 2021
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.

4 participants