UnitControl: Revamp support for changing unit by typing
#39303
Conversation
stokesman
added
[Type] Enhancement
|
Size Change: -40 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
| @@ -228,26 +177,20 @@ function UnitControl( | |||
| <Root className="components-unit-control-wrapper" style={ style }> | |||
| <ValueInput | |||
| aria-label={ label } | |||
| type={ isPressEnterToChange ? 'text' : 'number' } | |||
There was a problem hiding this comment.
As far as I can tell the use of 'text' is only to allow input of any character in Chrome (other common browsers don't limit input). With this PR that's no longer a concern. Removing this will default type to 'number' and have the possible benefit of easier validation since text inputs don't factor min, max, or step into validity.
There was a problem hiding this comment.
As far as I can tell the use of
'text'is only to allow input of any character in Chrome
This is an interesting change, to the point that I'd almost split it to a separate PR so that we can focus on testing it in isolation - WDYT @mirka ?
There was a problem hiding this comment.
I've reverted that from this PR but while working on the paste handling (in #40168 ) I found that robust/conventional support will require that the type be text. I left a review comment on that PR that may help elucidate.
stokesman
force-pushed
the
try/unit-control-alt-unit-switching
branch
4 times, most recently
from
e8d4927
to
594c5c4
Compare
stokesman
force-pushed
the
try/unit-control-alt-unit-switching
branch
from
594c5c4
to
42eb734
Compare
stokesman
changed the title
UnitControlBoxControl to send changes upon input
|
I'm not soliciting code reviews since this first needs vetting from a UX perspective. It would be awesome to have some folks test this out. I think @andrewserong and @aaronrobertshaw may be interested since they've made related issues/PRs; @ciampo and @mirka since this is components territory. Gutenberg.run is functioning again (as of this comment) so this PR can be tested there. To save a couple seconds, here's a Group block to that can be pasted to test with its padding <!-- wp:group {"backgroundColor":"secondary"} -->
<div class="wp-block-group has-secondary-background-color has-background"></div>
<!-- /wp:group -->Probably the first potential stopper—is it verboten to move focus in this manner? |
There was a problem hiding this comment.
Hey @stokesman , thank you for working on this exploration!
If we agreed on moving forward with the changes in this PR, what usecases are left for keeping the isPressEnterToChange prop on UnitControl / NumerControl / InputControl?
| @@ -228,26 +177,20 @@ function UnitControl( | |||
| <Root className="components-unit-control-wrapper" style={ style }> | |||
| <ValueInput | |||
| aria-label={ label } | |||
| type={ isPressEnterToChange ? 'text' : 'number' } | |||
There was a problem hiding this comment.
As far as I can tell the use of
'text'is only to allow input of any character in Chrome
This is an interesting change, to the point that I'd almost split it to a separate PR so that we can focus on testing it in isolation - WDYT @mirka ?
|
@mirka How exactly do I test this? Not sure where BoxControl is used. |
|
Hi @alexstine, BoxControl is likely most often encountered as the Padding control in the Dimensions panel in the block settings for a few blocks like Group and Columns. Unless you've already checked this branch out the easiest way to test would be at gutenberg.run/39303. If it helps, here's a group block to paste in to save a couple seconds: <!-- wp:group {"backgroundColor":"secondary"} -->
<div class="wp-block-group has-secondary-background-color has-background"></div>
<!-- /wp:group -->Also I believe the most crucial aspect of this to get your feedback on is the feature which allows changing the units of the control by typing the first character of one of the units while the quantity input is still focused. In this PR it will move focus to the select element immediately. That's opposed to way it works in trunk which doesn't move focus but requires that you type the units in and press enter or blur the input. |
|
@stokesman Here is what I did.
I tested the gutenberg.run link. Is something else supposed to happen? Thanks. |
|
Thanks for testing @alexstine and for your steps. I've changed step 4.
By following that you should experience the focus change we're trying to scrutinize. |
|
@stokesman I am happy with this. If a user understands enough to enter one of those characters, I think it only makes sense to move to the select field after. Thanks. |
|
I asked this before, but it likely got ignored because of every other comment that I left: If we agreed on moving forward with the changes in this PR, what use-cases are left for keeping the |
|
I hadn't forgotten about that question @ciampo. I couldn't think of a use case and thought maybe with a little time an idea would come to me. Yesterday, I searched in this branch did not see any. A search on plugins with WP Directory turns up some apparent use but I don't get a clear idea of the use cases by looking at those results. I'm thinking I might test a few of those plugins to try and find out because I'm curious. Also thank you for the quick review! |
stokesman
force-pushed
the
try/unit-control-alt-unit-switching
branch
from
036a644
to
f63dabd
Compare
I also can't personally think about any other reason. I found this comment which confirms our theory.
That would definitely be a good idea, let us know if/when you have any findings! It'd be nice if we could (of course, separately to this PR) simplify
Don't mention it! Thank you for your continuous efforts in contributing to the components package! Let us know once this PR is ready for another round of reviews |
stokesman
force-pushed
the
try/unit-control-alt-unit-switching
branch
from
f63dabd
to
9b1ac0f
Compare
|
I've gone ahead and marked this as ready and also made #40168 for the |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Reduce starting from the second unit and cover null safety
… the actions tab
ciampo
force-pushed
the
try/unit-control-alt-unit-switching
branch
from
86c1ccb
to
009d7c2
Compare
ciampo
force-pushed
the
try/unit-control-alt-unit-switching
branch
from
009d7c2
to
6a96ecd
Compare
There was a problem hiding this comment.
Rebased and made a bunch of improvements to unit tests, I think this PR is good to go 🎉
Thank you @stokesman for your work on this one !
There seem to be a few follow-up tasks:
- investigating the removal of all
isPressEnterToChangeusages from existing components (ie.BoxControl,DateTimePicker) (this should also help us with BoxControl: Add debounce to onKeyDown in underlying input controls #30222) - depending on how the previous task goes, we could then consider deprecating the
isPressEnterToChangeprop, and the behavior associated with it - add e2e tests to check the expected unit selection behavior, which can't be currently tested with
js-domas it seems like it doesn't support changingselectelement values using the keyboard - enhance the
onPastebehavior to also support unit change (work started in Add paste handling to #39303 #40168)
@stokesman , would you have any capacity to work on the above tasks? 🙏
|
Thanks for moving this one through Marco!
I can probably chip away at those. The first two would hopefully be pretty straightforward.
Because they'd cover behavior that the library doesn't control I wouldn't choose to write them. I mentioned the idea to say it could be done but not that it should be 🤷. Thanks again to all who tested and reviewed 🙌 ❤️ I'm fond of this little change. |
…39303) * Use a simpler way to allow changing units from the text input * Change default of `isPressEnterToChange` in `BoxControl` * Update tests * Avoid potential undefined * Remove extraneous null typing Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> * Make key matching regex case-insensitive Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Revert removed condtional type="text" on `UnitControl` * Forward __unstableStateReducer through `UnitControl` * Avoid intefering with paste or other shortcuts * Revert `BoxControl` changes * Update unit tests for `BoxControl` * Optimize and guard creation of regex Reduce starting from the second unit and cover null safety * Revise style * Make test of unit switching convenience run for each css unit * Add changelog entry * Remove custom `onChange` callback on Storybook, as it interferes with the actions tab * Improve tests by avoiding local `state` variables and using onChange spies instead * Clarify comments about js-dom not updating select inputs when using the keyboard * Update README --------- Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
What?
Revamps the type-to-change-units feature in
UnitControland removes it's dependency on theisPressEnterToChangeprop beingtrue.Also removes.BoxControl’s defaulting ofisPressEnterToChangetotruein order for it to send changes instantaneously (as opposed to now how it waits for either ENTER key press or ablurevent)Why?
Betters UX consistency in regard to inputs that produce changes instantly. Unlike all other text inputs in Gutenberg,With this PR, the feature works even when the mode of these controls set to instantaneous (BoxControl-based ones do not produce changes upon input. It seems the only reason for this difference is to enable the type-to-change-units feature which requires it.isPressEnterToChange={ false })The feature itself may also be found to work better but testing will tell. It's new implementation is certainly less complex and better from a maintenance perspective.
Related:
How?
Change of the defaultisPressEnterToChangeofBoxControltofalse.Testing Instructions
UnitControlin Storybook or Gutenberg.Screenshots or screencast
switch-units.mp4