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

Fix: range control direct entry in input field #25609

Merged
merged 8 commits into from
Sep 28, 2020

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Sep 24, 2020

The impetus is to fix #24460 which is a regression. The solution in this PR is simple and includes a test to prevent regressions but did require minor changes to quite a few tests. Also, though tangential to the issue, there is the removal of some extraneous code.

The core change is to InputControl, revising logic for value state updates and propagation. It no longer updates the internal value state from props while the input has focus. The change ensures that entry will be unimpeded by the validation logic of consuming components (such as RangeControl).

A new test is added for RangeControl specifying the allowance of invalid values in the input element until the loss of focus. It was confirmed to fail before the rest of the changes were added and so guards against future regressions.

Ancillary changes are the removal of RangeControl's wrapping of NumberControl and test updates to work with the new InputControl implementation detail.

How has this been tested?

  • Storybook
  • WordPress 5.5.1
  • Unit tests

Types of changes

  • Fixes: Spacer Block: Difficult to enter values directly into input field #24460, which despite the title affects RangeControl itself and all consumers of it.
  • Revised InputControl value state synchronization logic
  • Removal of extraneous wrapper around NumberControl in RangeControl
  • Added one test for RangeControl and changed one other.
  • Updated all tests involving InputControl, improving some by converting them to use react-testing-library.
  • Updated AnglePickerControl to avoid issue with new InputControl implementation detail

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [n/a] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • [n/a] I've included developer documentation if appropriate.
  • [n/a?] I've updated all React Native files affected by any refactorings/renamings in this PR.

@stokesman stokesman force-pushed the fix/range-control-direct-entry branch from 9edb601 to fb367a5 Compare September 25, 2020 03:29
@stokesman stokesman marked this pull request as ready for review September 25, 2020 04:16
@jasmussen jasmussen requested a review from talldan September 25, 2020 09:03
Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stokesman Wowow! Thank you so much for improving this. I had tried to improve the value sync (between uncontrolled and controlled) in the latest iterations of these controls (the base InputControl).

I think you cracked it by leveraging the blur event as the "trigger" mechanism to consolidate - very clever 👏 !!!

I left a comment in regards to the type for NumberControl within RangeControl. Not a huge thing at all.

Tested all of the affected controls locally.
Tested mouse + keyboard interactions as well as controlled and uncontrolled states.
It's working as expected!

Approved from me!

P.S. A part of me is still unsure about using the blur approach, but that's 100% due to my personal unfamiliarity with the technique - rather than something being wrong <3

@stokesman stokesman force-pushed the fix/range-control-direct-entry branch from fb367a5 to fe0563c Compare September 28, 2020 01:39
- on blur use handleOnReset if allowRest is true
- call onChange with a clamped value if new value is out-of-range
- update test to use invalid value instead of out-of-range value
- aside: remove an extraneous prop from number input
@stokesman stokesman force-pushed the fix/range-control-direct-entry branch from fe0563c to e211983 Compare September 28, 2020 02:17
@stokesman
Copy link
Contributor Author

stokesman commented Sep 28, 2020

I've pushed some updates because I found an issue with the value reset on blur happening when it shouldn't. Along with that, I realized I had some extraneous logic in there and what was probably overeager avoidance of onChange for out-of-range values. Also I've updated the readme.

@ItsJonQ, I'm glad you were added to review.

P.S. A part of me is still unsure about using the blur approach

I very much relate to that! I've gotten to be fairly confident it's good for this use case. I was also unsure about putting it in InputControl but besides how it simplied the sync logic, liked that it allowed removing the added layer of state management for the number input in RangeControl.

Along those lines, I've come to suspect the value state management for RangeControl is vestigial from before NumberControl was integrated. It seems that RangeControl would be perfectly fine to always be "controlled" now. I've given it a quick test as such and had success. Any thoughts on that? Not that we'd need to add it here.

@ItsJonQ
Copy link

ItsJonQ commented Sep 28, 2020

@stokesman I've tested it on my side, and it works as expected :).

The changes look good to me 👍 . If something unexpected just arise from this update, we can refine the implementation then! (or revert if we need and revisit how state syncing should be handled, haha)

Thanks again!

@ItsJonQ ItsJonQ merged commit 2d5106a into WordPress:master Sep 28, 2020
@github-actions github-actions bot added this to the Gutenberg 9.1 milestone Sep 28, 2020
@jasmussen
Copy link
Contributor

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spacer Block: Difficult to enter values directly into input field
3 participants