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

RangeControl component: disable when value is out of bounds #60633

Open
MaggieCabrera opened this issue Apr 10, 2024 · 6 comments
Open

RangeControl component: disable when value is out of bounds #60633

MaggieCabrera opened this issue Apr 10, 2024 · 6 comments
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@MaggieCabrera
Copy link
Contributor

Description

As reported by @afercia here, The RangeControl component right now is inconsistent with the value next to it if it allows for a different range than the slider does. This is happening right now on spacing controls like margin and padding, since the slider's range is 0-300 but the input allows any value to infinity (and negative numbers possibly soon). Changing the range of the slider is not possible so lets disable it when the value in the input is out of bounds.

Step-by-step reproduction instructions

  1. Insert a group block
  2. Change the margin to 4000, focus the RangeControl slider
  3. the slider shows the value is 300

Screenshots, screen recording, code snippet

319572841-cd4ea15a-2c04-4148-95bc-d3f1a0e51dd0

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@MaggieCabrera MaggieCabrera added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Apr 10, 2024
@mirka
Copy link
Member

mirka commented Apr 10, 2024

I discussed this with @DaniGuardiola for a second opinion, and we agreed that this a UI smell that tells us we're using a slider for something that shouldn't use a slider. Like the HTML attribute type="range" suggests, a slider UI is for selecting a value from a defined range, and is inappropriate for an input that doesn't have a defined range. @DaniGuardiola also noted that it could be extra confusing for a user that immediately interacts with the slider, and gets the wrong message that they're not allowed to choose values outside of that range.

If you still want to do some custom ad hoc overriding of component internals to achieve this (e.g. in BoxControl like you've mentioned earlier), let me know and I can help you with that. But we encourage y'all to reconsider showing the slider at all, since it goes against the standard mental model of what a slider is. And for that reason we probably don't want to bake this behavior into our components.

cc @WordPress/gutenberg-design

@mirka mirka added [Type] Enhancement A suggestion for improvement. and removed [Type] Bug An existing feature does not function as intended labels Apr 10, 2024
@mirka
Copy link
Member

mirka commented Apr 10, 2024

Also removing the Bug tag, since in the RangeControl component the number input and slider will always maintain the same range, as defined by the min/max props. The behavior and screenshot in the OP is a custom implementation that uses the RangeControl component in slider-only mode, which moves the responsibility of the value/range orchestration to the consumer.

@jasmussen
Copy link
Contributor

jasmussen commented Apr 11, 2024

a slider UI is for selecting a value from a defined range, and is inappropriate for an input that doesn't have a defined range.

The slider is a greatly ergonomical tool to set margins and paddings, I'd hate for us to have to remove it, that's why I think if you're intentionally typing in negative numbers, or very high numbers that break those bounds, disabling the slider would be a good way to allow both, and adhere to the spirit of the statement. Most of the time you shouldn't have huge values, or use negative values, that's why we'd require typing them in, and providing a bounded range is a useful additional guardrail.

I remember replacing headings with SVG fonts back in the day before web-fonts, and moving the actual text content -9999px to the left to still be legible by screen readers. This is a CSS practice that we want to be able to support. But it seems unreasonable to me to remove the slider as a result.

@afercia
Copy link
Contributor

afercia commented Apr 11, 2024

But we encourage y'all to reconsider showing the slider at all,

I agree with @mirka.
UI controls aren't absolute entities that are always good. They are good only when they correctly represent the value they are meant to control. Only in this case they provide a good user experience.

  • When a value is in a fixed range, the slider makes perfectly sense.
  • When the value does not have a fixed range, the slider should not be used in the first place. It's not the right UI pattern for that.

@jameskoster
Copy link
Contributor

Would it be strange if the range min/max updated based on the input value? So if you manually enter -999 then the range would be -999 to 300.

A provocation; should the order of the range / input should be visually flipped? If the slider is supposed to be the more ergonomic control, then shouldn't it be emphasised over the input, which is for edge case/custom values? If this is not the case then I tend to agree it's probably not adding a lot of value.

@MaggieCabrera
Copy link
Contributor Author

Would it be strange if the range min/max updated based on the input value? So if you manually enter -999 then the range would be -999 to 300.

Then 300 would still be a limit on the max value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

5 participants