NumberControl: Allow empty values #33485
Merged
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
aaronrobertshaw
added
[Type] Bug
This was referenced Jul 16, 2021
|
Size Change: +19 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Jul 16, 2021
There was a problem hiding this comment.
This is working nicely for me @aaronrobertshaw, tests pass, and it works according to the instructions in Storybook. I like that it's using the required param to enable the behaviour, too 👍
Fixes: #33319
Tiny nit here: this PR adds the ability to fix that one, but doesn't update the usage of the UnitControl in the BoxControl to add in required={ false }. Did you want to update the usage here, or once this PR lands, update #33444 to use the new param?
This is working nicely for me as-is, and the logic looks straightforward, so I'll give it the ✅.
Also CC: @aristath since you've done a bit of work on this recently, too 🙂
|
Thanks for taking a look at this 👍
Hopefully, this approach aligns it better with the HTML number input and makes it a little more intuitive.
Nice catch. I removed the updates to those components so that this PR could focus only on how the I think we can address updating those components and any others that need it once this gets merged. How they get updated might change based our what the final decision is regarding whether the Excluding them from #33444 also then means that PR only addresses a single issue as well. |
It would make sense if |
Add new prop to determine if NumberControl should allow empty string value.
To be more in line with an HTML number input, this changes the NumberControl to check the required prop to determine whether an empty string is valid or not.
aaronrobertshaw
force-pushed
the
update/number-control-allow-empty
branch
from
ef3d863 to
c3aab6d
Compare
Good point. I've updated the Now the
The UnitControl currently has fairly wide spread use:
All of these uses appear ok with the While testing these I did find an issue with clearing the FocalPointPicker control however it turns out it was already fixed in |
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.
Related: #33319
Description
This fixes an issue caused by the
NumberControleffectively enforcing a numerical value only via its use ofroundClamp.An example use case where an empty value would be valid is with the
BoxControlused for padding and margin block support styles. There is a need to be able to clear a field to an undefined value.Proposed solution
To bring the
NumberControlmore in line with an HTML number input this PR utilises arequiredprop to determine if an empty value can be applied via its state reducer. HTML number inputs, when not required, deem an empty value as valid.As the
NumberControlis currently experimental, the newrequiredprop will default to false. It is currently only used directly in a few places. It does form the basis for theUnitControlhowever from what I can see the existing uses ofUnitControlare ok allowing the field to be cleared.How has this been tested?
npm run test-unit:watch packages/components/src/number-control/testTest instructions
npm run storybook:devNumberControl, set a placeholder and toggle offrequired. Or, follow this link.NumberControl, using the keyboard delete the value, then press enter. The placeholder should remain after you click elsewhere.requiredknob back on and repeat step 4. The empty value will be replaced with0Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).