NumberControl: commit (and constrain) value on blur event
#39186
Conversation
ciampo
added
[Feature] Component System
|
Size Change: -6 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
ciampo
changed the title
InputControl: always commit value to internal state on blurNumberControl: commit (and constrain) value on blur event
|
Thanks for the ping @ciampo. This PR does as advertised but, as I see it, isn't a full fix for the issue. That's because this still calls In case you'd agree here's a test for the behavior I'm suggesting: it( 'should not call onChange callback with invalid values', () => {
const spy = jest.fn();
render(
<NumberControl
value={ 5 }
min={ 2 }
max={ 10 }
onChange={ ( v ) => spy( v ) }
/>
);
const input = getInput();
input.focus();
fireEvent.change( input, { target: { value: 1 } } );
expect( input.value ).toBe( '1' );
expect( spy ).not.toHaveBeenCalled();
} ); |
There was a problem hiding this comment.
This change is testing well for me manually, just left a small comment on the update action.
the query block would still have to guard against the out of range values in the onChange it supplies.
That's a good question!
I was wondering if there could also be cases where a consuming component should receive onChange calls while the user is typing (to display an error message or warning, or update the UI in some way), so we might not want to completely constrain it?
For consumers like the Query block that need to be that careful about values that are used, it could still be worth those consumers using their own validation for the appropriate values. Looking at the change back in #33285, I'd personally lean toward keeping the guards in the query block's toolbar there as they look like good defensive conditionals to me. But I might be overly wary of relying on component behaviour, so don't feel too strongly about it! 😀
ciampo
force-pushed
the
fix/number-control-improve-validation
branch
from
e664dba
to
e8b3d48
Compare
|
Thank you @andrewserong and @stokesman ! I should add, as a preface, that I'm not the original author of the Regarding the firing of
Personally I think that the best compromise is option is number 1 (with the addition of |
There was a problem hiding this comment.
NOTE: I had most of this review pending before e8b3d48 though it applies well enough still.
I took a closer look at this and suspected some unintended behavior. I added a console.log to the Storybook example and here's what I found:
The onChange with the constrained value does not fire until the input gains focus again. I've left a few notes on how to fix it up.
Still, I'm not convinced there is much value in this because constraint of an invalid value on blur is already a given in (typical) controlled contexts. That's because NumberControl intakes the value from props once it's no longer focused (given that valueProp !== value). It looks useful in the Storybook example because, while technically controlled, the wrapping component doesn't limit its own value.
|
Great to see the consideration of the options @ciampo. Regarding the 1st option and an The 2nd and 3rd options could also be combined. That is, the With regards to the excellent point raised by @andrewserong about the need of consuming components to respond to any input. It happens that |
|
Option 1 sounds good to me too! No strong opinion on whether to add a |
|
Thanks for the detailed follow-up @ciampo! These components are particularly tricky, and I know what you mean, it's hard getting into some of the details when we weren't the folks to originally write the code — I'd love it if longer-term we could reduce some of the complexity, too. Thanks for digging in!
Same, I like option 1, and the addition of an |
ciampo
force-pushed
the
fix/number-control-improve-validation
branch
from
e8b3d48
to
ee4336a
Compare
|
Thank you all for the last round of feedback — I should have fixed the issue with Regarding the behavior of the One thing that we may want to consider, finally, is whether consumers of Let me know what you all think! |
There was a problem hiding this comment.
This is testing well for me @ciampo, thanks for updating the Storybook example, too! (Tested a few UnitControl components within the editor, and the Storybook examples). ✨
I think that relying on native browser APIs here may be better on the long term (less custom code to maintain, less conventions).
That makes good sense to me!
Just left an optional comment about using the event object directly as the second param in the callback instead of wrapping in an extra object (so folks don't have to remember to destructure it), but I don't feel strongly about it, happy to go with whatever everyone thinks makes most sense here!
| The callback receives two arguments: | ||
|
|
||
| 1. `newValue`: the new value of the input | ||
| 2. `extra`: an object containing, under the `event` key, the original browser event. |
There was a problem hiding this comment.
I think I have a slight (though not at all strong) preference for having the second param be the plain event object rather than nesting it within an extra object. There's a familiarity with having the second param of callbacks be the event object itself, so it might make this component slightly easier for someone new to looking at it if we don't add in this abstraction.
But I'm aware you probably added it in mostly thinking of the isValid flag? I think given the documentation now covers event.target.validity.valid, we'd probably be fine not adding in any additional metadata.
What do you think?
There was a problem hiding this comment.
Chiming in to say the shape of that goes back well before this PR. I've suggested the same #33696 (comment). My feeling is the components are experimental so we have license to make the change as long as we fix up any internal stuff that might break. Good for a separate PR though.
There was a problem hiding this comment.
As @stokesman mentioned, the extra object existed before this PR and is defined in InputControl.
For example, UnitControl extends that extra object by adding a custom data property too (which seems to include extra info about which unit is being selected).
Given the above, I'm not sure what would be the best way to simplify the signature of the onChange callback — but it's definitely something that can be done in a follow-up
There was a problem hiding this comment.
Ah, excellent, thanks for linking the prior discussion, glad I'm not alone there! 😄
Yes, I think that's perfectly fine for a follow-up, not a blocker for me!
| setIsValidValue( extra.event.target.validity.valid ); | ||
| } } | ||
| /> | ||
| <p>Is valid? { isValidValue ? 'Yes' : 'No' }</p> |
There was a problem hiding this comment.
Nice way of visualising this in Storybook 👍
ciampo
force-pushed
the
fix/number-control-improve-validation
branch
from
d91e6a4
to
1c01d6d
Compare
|
Just rebased on top of |
|
As I was giving a final look at the PR after rebasing, I found an issue with the current approach: Basically, when dragging gestures, the number-control-drag.mp4This causes 2 issues:
I'm not sure if we can (or want to) change the behaviour of We should probably update our types (and docs) to reflect the correct types for the Regarding the
Thoughts? |
I have to agree it'd be better to expose only what we decide to.
Sounds good to me. I'd say feel free to drop my commit right out of history. I would have done so but didn't want to force push on this branch. |
…clamped on `blur`
…ly not being defined
ciampo
force-pushed
the
fix/number-control-improve-validation
branch
from
b0da91f
to
dad09a5
Compare
|
Thank you, @stokesman! I dropped your commit as agreed. I believe this PR is ready for a final check |
There was a problem hiding this comment.
Re-tested, this is looking good to me @ciampo! Min/max constrained values are working correctly, both in Storybook examples, and in the editor (specifically looked at the Spacer height control since we were already looking at that separately). And the tests are passing 👍
LGTM!
…ess#39186) * `InputControl`: always commit value to internal state on `blur` * `NumberControl`: constain value also on the `UPDATE` action * CHANGELOG * Add unit test * Remove `UPDATE` action from `InputControl` s state reducer * Add unit test to check that `onChange` gets called when the value is clamped on `blur` * Fix input control blur logic, so that `onChange` is called after clamping the value * Comments * Use `event.target.validity.valid` in docs, storybook and unit tests * README * Add docs and update example about input validity * Update `onChange` event type * Override `event.target`, update types * Revert docs and Storybook changes about `target.visibility` potentially not being defined
Description
Flagged in #33285
Closes #33291
Changes in this PR:
InputControlcomponent when losing focus (blurevent):isPressEnterToChangeprop was set totrueNumberControl's internal state reducer constrains the incoming value also when receiving anUPDATEactionThis change allows the
NumberControlvalidation (which is already in place) to kick in when the input loses focus.Testing Instructions
On
NumberControl:minandmaxvaluesminandmaxboundstabkey to shift focus away from the input)The unit test added in this PR:
InputControlandNumberControl, and notice how the test failsCheck, in Storybook, that all usages of
InputControlandNumberControlwork as expected (egUnitControl,BoxControl,RangeControl...)Check that all usages of
NumberControlin Gutenberg work as expectedMake sure that all unit tests continue to pass
Screenshots
Before:
number-control-before.mp4
After:
number-control-after.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).