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

BoxControl: Add debounce to onKeyDown in underlying input controls #30222

Open
aaronrobertshaw opened this issue Mar 25, 2021 · 1 comment
Open
Labels
[Feature] Inspector Controls The interface showing block settings and the controls available for each block [Type] Enhancement A suggestion for improvement.

Comments

@aaronrobertshaw
Copy link
Contributor

What problem does this address?

Currently the BoxControl uses the isPressEnterToChange prop on its underlying unit controls. This isn't consistent with a lot of other uses of UnitControl.

It appears that it may do this to prevent a jumpy user experience when editing values such as margins or padding. The downside is that a lot of users may not understand that they need to press enter to commit the change or select a new field so the blur event can trigger the onChange.

An example of such a situation can be found in this discussion on a PR adding margin support to the separator block.

What is your proposed solution?

  • Remove the use of isPressEnterToChange for the BoxControl to improve consistency of UX
  • Add a debounce to the underlying onKeyDown handler to smooth out the UX as well
@aaronrobertshaw aaronrobertshaw added [Type] Enhancement A suggestion for improvement. [Feature] Inspector Controls The interface showing block settings and the controls available for each block labels Mar 25, 2021
@stokesman
Copy link
Contributor

stokesman commented Mar 29, 2021

I tend to agree that the UX would be improved without isPressEnterToChange and it'd probably be fine to remove it for now.

The caveat is that it's intended to allow changing units by typing. E.g. typing 10% + Enter can change the units to percent (source: #25933 (comment)). Though I don't know anywhere that's currently applicable other than in Storybook. Even when I've specified the units for spacing in experimental-theme.json they aren't available in BoxControl in the editor (and I don't know if that's an issue or if I'm missing something). So while in concept it is a nice UX affordance, it doesn't make sense to have its mere potential blocking the suggestion in this issue. Also, I think it could be possible to provide without requiring isPressEnterToChange to be true.

The debounce might be a good idea I'm not sure there'd be a problem without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inspector Controls The interface showing block settings and the controls available for each block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

2 participants