fix(number-field): add support for modified stepping #1534
Conversation
| } | ||
|
|
||
| private shiftPressed = false; |
There was a problem hiding this comment.
Thought on leveraging https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/shiftKey as opposed to hanging additional state off of the element?
You likely adopted this from the fact that it isn't used in the Color elements, but likely should be. Here where it is just the one, it seems like a good idea.
There was a problem hiding this comment.
Weird! For some reason I didn't see the test file for Numberfield, I remembered it being empty, but thats obviously not the case. I'll get on it!
In the meantime, the shift key is tricky. So here's the deal - I thought it should work not only for arrow key input, but also the stepper buttons and for scrubbing in my other PR as well.
For these other modalities, we're relying on PointerEvents. I really just assumed that shiftKey couldn't be queried from a non-keyboard event, but I guess I'm wrong. Even so, I don't see the property documented on MDN, and I'm wondering how global and reliable it is.
Secondly, the stepper buttons operate on a timer, so aren't even triggered by a proper event. I might be able to nest the PointerEvent in the closure, but setting state just seemed more natural and straightforward.
Lemme know what you think!
There was a problem hiding this comment.
Oops, scratch that first point - I just didn't look down the inheritance chain to MouseEvent. But I think my second point stands.
There was a problem hiding this comment.
So, forget everything I said. Here's what happened...
I realized that if the modifier key is going to be here, it should apply to all situations including the scroll wheel which I did not cover. Unfortunately, I found the scroll a bit crazy. I can scroll a bit and suddenly find myself in the 100k's or even millions. This issue seemed to have the same complaints if I were to guess.
Your demo to the person who filed the issue seems exactly inline with how I solved, except the shift kills the input (OK for you, not for this PR though). Since you don't have a scroll wheel, I'll tell you that the shift key modifies the axis of input. When before you got deltaY, with shift held you get deltaX.
Anyway, that's my functionality change here in the latest.
I also found that my shift state that you called out wasn't being set properly for when I was scrolling. Not sure why, but I just took that as a sign to kill it. I ended up carrying the pointer events throughout the timed change requests for the stepper buttons.
In terms of testing, I added 4 tests. One each for default shift Arrow Up/Down, and one each for changing step and the step modifier values on Arrow Up/Down.
I also needed to alter a couple other tests because they were using the ArrowKey + Shift as a way to clear out the field and start over.
Lastly, I tried to make some tests around Shift + stepper buttons/scroll wheel, but wasn't sure how to send button/wheel events at the same time as a shift key. I'm assuming that what I already did is probably good enough, though.
…ey to block and clear input
|
@bengfarrell sorry for the delay on this. Would you be able to clean up this one conflict? Then I'll merge this down into the holding branch and make sure the tests/et al are passing and we can get this into next week's release! |
Sure thing! Done. The only difference in the new logic was obviously the shift modifier, but also checking that the deltaX/Y is not 0 before running the stepBy function. So I used my logic, and added that check back in |
Westbrook
changed the title
* feat(number-field): add shift modifier to multiply key input * feat(number-field): put stepper buttons in shift modifier path * fix(number-field): change scroll behavior to something more sane, add/modify tests * test(number-field): fixed some tests that were relying on the shift key to block and clear input
* feat(number-field): add shift modifier to multiply key input * feat(number-field): put stepper buttons in shift modifier path * fix(number-field): change scroll behavior to something more sane, add/modify tests * test(number-field): fixed some tests that were relying on the shift key to block and clear input
* feat(number-field): add shift modifier to multiply key input * feat(number-field): put stepper buttons in shift modifier path * fix(number-field): change scroll behavior to something more sane, add/modify tests * test(number-field): fixed some tests that were relying on the shift key to block and clear input
* feat(number-field): add shift modifier to multiply key input * feat(number-field): put stepper buttons in shift modifier path * fix(number-field): change scroll behavior to something more sane, add/modify tests * test(number-field): fixed some tests that were relying on the shift key to block and clear input
Step multiplier for faster stepping
Description
Added step multiplier - this will multiply the step when holding down shift. Originally I had this working for just the arrow keys, but added support for pressing the stepper buttons as well. The multiplier is a settable attribute called "step-modifier".
We had discussed allowing any modifier, but instantly I noticed that CTRL + ArrowUp does a spaces thing on OSX, so I'm limiting to just shift right now.
Related Issue
#1528
Motivation and Context
Motivation is largely for my project which includes a canvas where objects on canvas have transforms. Stepping by 1px at a time is super slow, and our designer wanted a way to increase that 10x.
How Has This Been Tested?
Just lightly tested in Chrome as I was developing
Screenshots (if appropriate):
Types of changes
Checklist: