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 component only allows adjusting in increments of 10 via drag gesture #2117

Closed
iamthomasbishop opened this issue Apr 6, 2020 · 10 comments · Fixed by WordPress/gutenberg#21451 or #2119
Assignees
Labels
[Type] Bug Something isn't working

Comments

@iamthomasbishop
Copy link
Contributor

Describe the bug
Slider component only allows adjusting in increments of 10 via drag gesture.

To Reproduce
Steps to reproduce the behavior:

  1. Add a Spacer block
  2. Open block settings by tapping on the cog/gear icon
  3. Try dragging to adjust the height of the block
  4. Notice that you can only adjust in increments of 10

Expected behavior
I expect to adjust by increments of 1.

Screenshots
RPReplay_Final1586206890-min 2020-04-06 16_03_41

Smartphone (please complete the following information):

  • Device: iPhone 11 Pro
  • OS: iOS 13.3.1
  • Version: Tested via build 26428 via this PR (cc @lukewalczak), but I'm also seeing this issue on the latest primary internal beta. Not seeing this on production, so it must have been a recent change.
@iamthomasbishop iamthomasbishop added the [Type] Bug Something isn't working label Apr 6, 2020
@pinarol
Copy link
Contributor

pinarol commented Apr 6, 2020

@lukewalczak Could you check what’s going on here? Let’s try to fix this one as soon as possible.

@lukewalczak
Copy link
Contributor

It looks like it's the effect/result of PR related to sharing edit method with web, where step was unified and set to 10 probably to match the web behaviour.
If there is no reason to keep that I will suggest using Platform API and set the correct step value according to platform e.g:

import { Platform } from '@wordpress/element';

...
<RangeControl
	...
	step={ Platform.OS === 'web' ? 10 : 1 }
/>

@iamthomasbishop
Copy link
Contributor Author

Ah, okay, thanks for investigating @lukewalczak! Maybe I'm missing some rationale that the web folks have for single increments to be 10px vs. 1px, but it feels really odd to me (even on the web, but esp in the context of a slider on mobile). Are we able to override that to apply increments of 1?

@lukewalczak
Copy link
Contributor

lukewalczak commented Apr 6, 2020

I didn't take part in refactoring Spacer block, so it looks incorrectly for me as well!

Are we able to override that to apply increments of 1?

Sure, my suggestion above is fixing that in this way (mobile: step = 1, web: step = 10),however let's wait for @pinarol opinion on that ✌️

@iamthomasbishop
Copy link
Contributor Author

Sure, my suggestion above is fixing that in this way (mobile: step = 1, web: step = 10),however let's wait for @pinarol opinion on that ✌️

Sounds good! To be fair, it is a nice thing to have the ability to set step values (particularly something that has a smaller range but a Stepper isn't desirable — like image sizes or something along those lines), but the default unit, imo, should be 1.

FYI @SergioEstevao (who worked on the Spacer refactoring recently, although I don't think he changed anything w/ this step value 😄)

@lukewalczak
Copy link
Contributor

Sounds good! To be fair, it is a nice thing to have the ability to set step values (particularly something that has a smaller range but a Stepper isn't desirable — like image sizes or something along those lines), but the default unit, imo, should be 1.

Agree on that!

@pinarol
Copy link
Contributor

pinarol commented Apr 7, 2020

Sure, my suggestion above is fixing that in this way (mobile: step = 1, web: step = 10),however let's wait for @pinarol opinion on that ✌️

Sounds good to me considering that @iamthomasbishop is giving the 👍

@SergioEstevao
Copy link
Contributor

I defer to you @iamthomasbishop but I don't think it makes sense increments of 1, do you think our users will increment/want spacers with 1px difference?

Can we check with the web folks what is the rationale there? Or you already checked with them?

@SergioEstevao
Copy link
Contributor

I talked a bit with @pinarol and @lukewalczak and I changed my mind because of two things:

  • On the web there is the option to directly manipulate the spacer size,
  • and also write a custom value, so we can have values that are different from multiples of 10

So I agree that we should have steps of 1 on mobile.

@iamthomasbishop
Copy link
Contributor Author

I talked a bit with @pinarol and @lukewalczak and I changed my mind because of two things:

👍 Sounds good. It'd still make sense to touch base w/ the web folks to see what their rationale may have been.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something isn't working
Projects
None yet
4 participants