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

Components: update the rounding logic for shift key increments in NumberControl and RangeControl #34817

Open
4 tasks
Tracked by #34345
ciampo opened this issue Sep 14, 2021 · 5 comments
Open
4 tasks
Tracked by #34345
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@ciampo
Copy link
Contributor

ciampo commented Sep 14, 2021

What

Update the rounding logic for when the user increments/decrements the value in NumberControl and RangeControl components while holding the shift key.

Currently, the result of a shift increment/decrement is rounded to the nearest multiple of the shift step value.

Current behaviour Target updated behavior
current target

Why

Make the shift increment more predictable for the NumberControl and RangeControl components by applying the shift step increment/decrement with the same rounding as the base step operation.

A/C

  • Apply the same rounding as the base step operation to the shift step increment/decrement
  • After Directly handle shift-stepping in RangeControl #34719, NumberControl is the only component using the useJumpStep hook. We should consider removing that function, and replace it with a new utility that enables sharing the new shift-stepping, rounding logic between NumberControl and RangeControl
  • Consider updating both component's READMEs (if necessary)
  • Consider adding an entry to the components package's CHANGELOG
@ciampo ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. labels Sep 14, 2021
@ciampo
Copy link
Contributor Author

ciampo commented Sep 14, 2021

Hey @stokesman , can you please check this issue's description and confirm it's in line with what you initially described in #34542 ?

@stokesman
Copy link
Contributor

Thanks for making the issue @ciampo 👍 It captures the intent. I put in the minor edit to say the shift-step will be applied with the same rounding as the base step.

@ciampo
Copy link
Contributor Author

ciampo commented Oct 11, 2021

Update: after #35020 and #34542, the situation is as follows:

  • the shift-stepping functionality has been removed from RangeControl — which means that only NumberControl currently has it
  • the useJumpStep has become dead code, since NnumberControl also stopped using it

We should now think about the next steps:

  • We should probably remove the useJumpStep functionality (it can always be added back later on)
  • We should consider the next steps around shift-stepping:
    • We could drop it from NumberControl as well
    • We could look into re-adding it to RangeControl
    • We could keep things as they currently are
  • Whatever decision we make, I believe that we should first focus on improving the stability of these components:
    • We could refactor them to TypeScript
    • We should write more tests

Tagging here a few folks that may want to share their opinion here: @jasmussen @diegohaz @mirka @jorgefilipecosta @ntsekouras @aaronrobertshaw @andrewserong

@andrewserong
Copy link
Contributor

Thanks for the ping and for writing up the current state of things!

We could refactor them to TypeScript
We should write more tests

Personally I'd prioritise these over following up with the shift-stepping behaviour in NumberControl.

For next steps around shift-stepping, I'd lean toward keeping things as they currently are. I don't think it's worth the trouble adding it back into RangeControl (since it doesn't really seem that beneficial for that component), and if the behaviour is currently working okay in NumberControl, there probably isn't any urgency in removing it. That said, if it isn't something used by users, it's nice to remove unnecessary code and simplify things where we can. But I don't feel strongly about it if anyone has other preferences 🙂

We should probably remove the useJumpStep functionality (it can always be added back later on)

That sounds like a good idea to me 👍

@ciampo
Copy link
Contributor Author

ciampo commented Oct 12, 2021

Personally I'd prioritise these over following up with the shift-stepping behaviour in NumberControl.
[...]
For next steps around shift-stepping, I'd lean toward keeping things as they currently are

I personally agree with both opinions.

Next steps then look like:

  1. Remove useJumpStep — done in Components: remove unused useJumpStep utility #35561
  2. Improve NumberControl and RangeControl by:
    • Rewriting in TypeScript
    • Hooking to the Context System
    • Using Emotion for styles
  3. Add more tests to NumberControl and RangeControl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

3 participants