Support "any" step in NumberControl and RangeControl #34542
Conversation
|
Size Change: -10 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ciampo
added
[Feature] Component System
| modifier | ||
| ); | ||
| if ( delta !== 0 ) { | ||
| delta = Math.ceil( Math.abs( delta ) ) * Math.sign( delta ); |
There was a problem hiding this comment.
The value of delta comes from react-use-gesture and is a decimal value. It's converted to an integer in order to preserve the NumberControl's decimal value as entered (when step="any"). Done via Math.ceil etc. because single pixel movements produce values like:
|
Hey @stokesman , thank you for your PR!
Would you be able to point me to where this is stated? In the README, the
While I understand that this may be a more predictable behaviour for you, I'll have to look more into it and consider if this is a change that we'd want to make to the component. It may be confusing to users that got used to the current rounding logic and therefore be seen as a sort of breaking change. |
|
Thanks for taking a look and for the feedback @ciampo.
I wrote "allows
I believe that most users of the feature only even know about it because they use the same feature in other applications. All other applications I know with the feature, do not round by an increased amount while holding Shift. So for those relatively advanced users, I think we'd avoid frustration with the unique implementation we currently have. |
There was a problem hiding this comment.
@stokesman Thanks for working on this! 👍
The updated NumberControl works as advertised.
Personally, I like the consistent and predictable application of step values. That said, I can appreciate that this is a change away from the current behaviour that some users may not perceive as a bug and have grown accustomed to.
The docs for the shiftStep prop don't seem to communicate any rounding of the value based off of it. So perhaps that aspect of the changes in this PR are more in line with the advertised functionality?
Amount to increment by when the SHIFT key is held down. This shift value is a multiplier to the step value. For example, if the step value is 5, and shiftStep is 10, each jump would increment/decrement by 50.
Regarding the support of "any" I'll happily defer to @ciampo on that. 🙂
There was a problem hiding this comment.
About supporting step="any"
The README also states that "NumberControl is an enhanced HTML input[type="number] element". Was lack of support for step="any" intended as an enhancement then?
Seen in this light, it does look like adding support for any as a valid value for the step prop would increase this component's adherence to the HTML input spec.
It would be good, then, to update the README:
- the type of the
stepprop should become something likenumber | "any" - there should be a short paragraph explaining what to expect when setting
step="any"
It'd be also good to add support for any to the Storybook example
About the shift step rounding changes
@aaronrobertshaw made a very good point:
The docs for the shiftStep prop don't seem to communicate any rounding of the value based off of it. So perhaps that aspect of the changes in this PR are more in line with the advertised functionality?
So I'd say that we can go ahead and implement this change — but the approach I would take would be slightly different: I would instead look at changing the logic directly in the useJumpStep function.
Splitting across two separate PRs
Since the step="any" attribute is also supported by <input type="range" />, similar changes to the ones applied to NumberControl in this PR should be also applied to RangeControl.
Also, since useJumpStep is also used by RangeControl, I would propose that:
- We keep this PR focused on adding support of the
step="any"prop to bothNumberControlandRangeControl - We make changes to the shift step rounding logic in a separate PR, by changing the
useJumpStepfunction
stokesman
force-pushed
the
fix/number-control-any-step-and-shift-rounding
branch
from
e24fa9a to
62e73d0
Compare
|
Thank you both @aaronrobertshaw and @ciampo 🙇
I like that idea and have pushed some changes in that direction. I've yet to add any changes for
I'm all for the separate PR and sharing any appropriate abstractions but, if we change |
stokesman
changed the title
|
Thanks for the testing and feedback Marco! I've gone ahead with most of the suggestions you made.
Unless it was partly due to the mistake I'd made (and you caught #34542 (comment)) then I believe all that is existing and what this PR is helping us get closer to fixing. I have #34566 waiting in the wings that adds some tests and fixes for
I think that is #33291 for which #34128 exists. (Also not sure if you expected the UA to do it. None that I know of do that for number inputs just range inputs (and that's only Firefox as far as I know)). |
|
I just went through and tested on the github hosted storybook and could reproduce exactly the results described in #34542 (review). Those are expected in that they are what we currently ship. We do have some unit tests that should fail given any shift-step changes (they did in the original version of this PR which included shift-step rounding changes). |
There was a problem hiding this comment.
I just went through and tested on the github hosted storybook and could reproduce exactly the results described in #34542 (review). Those are expected in that they are what we currently ship
I can confirm that the behaviour that I flagged previously can be also replicated in production — so we shouldn't count it as a blocker for this PR.
We do have some unit tests that should fail given any shift-step changes (they did in the original version of this PR which included shift-step rounding changes).
That's a good point (although they were not testing for the bug that I flagged earlier).
In that regard, thank you for flagging #34566 and #34128 — it'd be great if we could work on it as a follow-up, I believe those fixes and the additional tests are much needed, and I would argue they should take precedence over resuming the work in #34817.
I was ready to approve this PR, but I decided to give it a final look — and that's when I discovered another unexpected behaviour, which I believe should be fixed in this PR since it's related to step="any".
Steps to reproduce:
- I set
step="any" - I set the value of the input to a non-integer number, e.4.
4.3 - I increment/decrement the value via keyboard, works as expected
- I press
enterto force the rounding — the.3part of the number is preserved, as expected - I click-drag, the number increments/decrements as expected
- I click on the up/down arrows on the right side of the input — the value gets rounded to the closest integer. I think this behaviour is unexpected and should be updated to reflect the rest of the interactions listed above — what do you think?
Here's a screencast of the behaviour that I just described
Kapture.2021-10-01.at.12.56.54.mp4
Finally, I think it would be nice if we could add tests specific to steps="any" (as part of this PR).
stokesman
force-pushed
the
fix/number-control-any-step-and-shift-rounding
branch
from
234ff0e to
a041308
Compare
|
Thanks for testing this once again @ciampo.
I agree and it presents a bit of a dilemma. We can't (easily) have both shift-step working for clicks on those up/down arrows and have them behave as they should natively in regards to not rounding "any" steps. It's because the shift-step for those arrows happens the same way it did for I've pushed a change for what I think is the sensible way to proceed and that is removing the mutation of the step attribute. So clicking the arrows while holding shift won't shift-step. We could compromise and remove it only when One more related note is it looks like we may be adding our own stepping arrows to I've added some tests for |
There was a problem hiding this comment.
Given that the component is experimental and that it seems unlikely many folks expect to shift-step while clicking those arrows, I think it's fine to remove it. Additionally, mutating the attribute may not cause any known issues but it's a bit dirty as it can temporarily invalidate the input.
One more related note is it looks like we may be adding our own stepping arrows to NumberControl in which case having them shift-step would be relatively easy.
Thank you for the detailed explanation. I agree with the whole reasoning here.
I can also confirm that the issue that I flagged in my previous comment is now fixed.
I've added some tests for step="any" with NumberControl to the PR. They are only for keyboard input because testing the drag or "spin" arrows in jsdom would be a fancy trick. I looked at adding some tests for RangeControl but can't see there's any worthwhile tests to make there.
Thank you for this as well.
I am not 100% sure I like how we're hiding the number input in the RangeControl component when step="any", but it's probably something we can iterate on in a separate PR as a follow-up.
Let's merge this 🚀 (and thank you again for the patience while working on this PR)
This eliminates the only thing useJumpStep was affecting so the hook is removed from NumberControl.
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
stokesman
force-pushed
the
fix/number-control-any-step-and-shift-rounding
branch
from
a041308 to
f39dc37
Compare
|
Thank you @ciampo 🙇 I just pushed one more commit for a changelog entry (on a rebase). A test may have flaked but as soon as I can get it all green, I'll merge it. One other thing, It looks like |
Thank you for flagging it. Let's continue this conversation in #34817 |
Closes #34816
Currently,
NumberControlallowsstep="any"but attempting to use the arrow keys or dragging to change the value results inNaN. This PR fixes that.Support in
RangeControlpresents a bit of a design challenge #34542 (comment) and alternate ideas on resolving it are welcome.How has this been tested?
Unit tests and manually comparing behavior of
NumberControlwith behavior of aninput[type="number]element using the samestepvalues.Screenshots
NumberControlsupport of "any" step with arrow keys and draggingRangeControlswitching betweenstepof 1 and "any"range-control-step-1-or-any.mp4
RangeControlmarks with "any" stepThe mark handling code needed a tiny revision to treat
step="any"as if the step was 1. While testing that I noticed that the styling for marks has regressed but it's a separate issue (#35153).range-control-step-any-with-marks.mp4
Types of changes
enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).