InputControl: Allow onBlur for empty values to commit the change, move reset behaviour to ESCAPE key #39109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Enhancement
|
I thought I'd have a go at this one — it looks like it might not be as simple as it looked superficially — we also need to support being able to type in the unit, and switching this flag to |
|
Size Change: +13 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
andrewserong
changed the title
|
Thanks for working on this and all the copy+paste commands in the test description 😄 I tested the padding/margin/blockGap inputs (and linked sides where available) in both the block and site editors. ✅ Hitting
|
Interesting, good catch @ramonjd! I think that might be because the component is unmounted before |
ramonjd
reviewed
Feb 28, 2022
What I forgot to say is that it didn't really stand out to me as an issue. Being able to edit values using the keyboard and via mouse clearing is already a huge win. |
ciampo
approved these changes
Mar 1, 2022
There was a problem hiding this comment.
Excellent work, @andrewserong (and thank you for the ping!) I think this change is an improvement over the previous behaviour 🚀
Before
box-control-before.mp4
After
box-control-after.mp4
Regarding the onBlur callback not being fired when unmounting the component, I've found a potential workaround (although it's definitely something that can happen a follow-up, since it's not really related to the changes in this PR) highlighted in facebook/react#12363 (comment)
mirka
approved these changes
Mar 1, 2022
|
I think the changes here look fine but I am wondering if the option to stop using |
|
Thanks for the reviews and feedback, folks!
Good question — I took a look at the existing tests and the tests at the
That's a great question, too. I had a go at switching it off before opening this PR, but noticed as you mention in that issue (#30222 (comment)) that it then switched off the behaviour where you can type in the unit and then press Enter to switch units. Without knowing whether or not anyone currently relies on that option, I thought I'd open this PR in the shorter-term to at least improve the UX of this mode. But I think it'd be worth exploring if folks think it's worth it to switch it off on the BoxControl! |
andrewserong
deleted the
try/update-dimensions-controls-not-to-require-enter-to-commit-value
branch
stokesman
reviewed
Mar 2, 2022
| @@ -13,6 +13,7 @@ | |||
|
|
|||
| - `TreeGrid`: Add tests for `onCollapseRow`, `onExpandRow`, and `onFocusRow` callback functions. ([#38942](https://github.com/WordPress/gutenberg/pull/38942)). | |||
| - `TreeGrid`: Update callback tests to use `TreeGridRow` and `TreeGridCell` sub-components. ([#39002](https://github.com/WordPress/gutenberg/pull/39002)). | |||
| - InputControl: Allow `onBlur` for empty values to commit the change when `isPressEnterToChange` is true, and move reset behavior to the ESCAPE key. ([#39109](https://github.com/WordPress/gutenberg/pull/39109)). | |||
There was a problem hiding this comment.
Late comment but this missed the Unreleased section (easy to do when it's empty). Maybe someone can move it in another PR before the release.
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
As reported in #39090, the existing default behaviour of the BoxControl (as used in Padding and Margin dimensions controls) where you need to press Enter to commit a value change such as clearing out the padding value, can be confusing for users when it comes to clearing out the value.
Prior to this PR, when you blur (e.g. tab or click) out of the UnitControl (with
isPressEnterToChangeset to true) when it contains no value, this will be treated as cancelling / resetting the current change.After this PR, we treat any change to the input field (even it's empty) as a change to commit when tabbing or clicking out of the field.
To preserve the ability to cancel the current change, the
reset()behaviour is moved to the ESCAPE key, which has some consistency with the W3C WAI recommendations for editing within a data cell (namelyEscape: if content was being edited, it may also undo edits) — it's not an exact match to the use case of this component, but I think there's a close enough parallel.The proposed change here is an opinionated one, but hopefully it helps alleviate some of the frustration with editing Padding and Margin values as in the initial report.
Changes included:
isPressEnterToChangeis set on the InputField, commit empty valuesisPressEnterToChangeis set on the InputField, allow theESCAPEkey to cancel the current (not-yet-committed) changeTesting Instructions
10pxor2remor5%and then press enter and check that it still works correctly.Confirm existing tests pass (you can look at the CI job, but I ran the following locally to confirm the tests were passing):
Also check that the components still behave correctly in Storybook (via
npm run storybook:dev). Test the BoxControl component in particular: http://localhost:50240/?path=/story/components-experimental-boxcontrol--defaultScreenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).