Update NumberControl stepping to match HTML number input stepping
#34566
Conversation
This file contains hidden or 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
|
Size Change: +2 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
7 tasks
380d388 to
0d12c6a
Compare
8 tasks
6 tasks
8 tasks
f08bd34 to
701b64d
Compare
|
Hi, @stokesman Is this still relevant draft PR? |
- Rounds to steps starting from min - Uses rounding precision determined by the greater of min or step - If max is not on a step, uses the closest step less than max
701b64d to
b85a20d
Compare
Thanks for checking @Mamaduka. It still looks relevant to me. I rebased and updated the description to include some links to Storybook where the issue can be easily reproduced. Tests are passing so I’ll mark this as ready. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
stokesman
commented
Mar 19, 2025
Comment on lines
85
to
82
| min = Infinity, | ||
| max = Infinity, | ||
| step = 1 | ||
| ) { | ||
| export function ensureValidStep( value = 0, min = Infinity, step = 1 ) { |
There was a problem hiding this comment.
Just noticed that min = Infinity seems to make no sense and I left that unchanged. Given that the proper stepping behavior requires starting from min a real number has to be used. This is currently accounted for on L86 but this looks like it can be simplified by just defaulting to zero to begin with, i.e. min = 0.
There was a problem hiding this comment.
Actually, I decided none of the defaults are sensible. I removed them in 80ae1db. There’s only a single consumer of this and it’s specifying all the arguments.
Maybe this shouldn’t even live in the component package’s top level utils folder. I guess it’s here because roundClamp was also used in FocalPointPicker. Happy to move it somewhere into number-control if anyone feels it’s a good idea.
There was a problem hiding this comment.
No strong opinions from my side
stokesman
commented
Mar 20, 2025
| * @param {number} value The value. | ||
| * @param {number} min The minimum range. | ||
| * @param {number} max The maximum range. | ||
| * @param {number|string} value The value. |
There was a problem hiding this comment.
Unrelated—pardon the detour. I noticed value can be a string. It’s in the unit tests for this function:
gutenberg/packages/components/src/utils/test/math.js
Lines 55 to 59 in b85a20d
It’s notable that the final assertion tests a string value for
max too which apparently works, but TS would complain if we tried to update the type since it says Math’s min and max don’t support string.
I suppose this file and its unit tests ought to be refactored to TS.
There was a problem hiding this comment.
I suppose this file and its unit tests ought to be refactored to TS.
That would be great — ideally all code in the components package should be written in TS
ciampo
approved these changes
Mar 24, 2025
Mamaduka
approved these changes
Mar 24, 2025
There was a problem hiding this comment.
Everything seems to be in order when I tested this manually ✅
Thanks, @stokesman!
NumberControl stepping to match HTML number input stepping
chriszarate
pushed a commit
to chriszarate/gutenberg
that referenced
this pull request
Jul 1, 2025
…ordPress#34566) * Update roundClamp to match HTML number input stepping behavior - Rounds to steps starting from min - Uses rounding precision determined by the greater of min or step - If max is not on a step, uses the closest step less than max * Rename and decruft `roundClamp` * Restore offset by `min` * Fix tests * lint * Remove defaults; condense code style * Add changelog entry Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
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.
At present, it's easy to create a
NumberControlthat steps (using keyboard arrow keys or dragging) to invalid values. This is in contrast to the Standard HTML<input type="number"/>which does not step to invalid values. This PR modifies (and renames) theroundClamputility function in order to have it avoid invalid values:minminorstepThese issues can be reproduced in Storybook by stepping through some values. Observe the ”Is valid?” output as seen in this screenshot:
Links to configurations of NumberControl that reproduce this:
minminvalueHow has this been tested?
Manually with
NumberControlusing various combinations ofmin,maxandstepand comparing the behavior of a plain<input type="number"/>with the same combinations (in Chromium).Screenshots
Steps starting from
minRounding to the greater precision of either
minorstepTypes of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).