Allow continuous change of numeric input value with button press #2174
Conversation
simulate button press using "mousedown"Preview: documentation | landing | table |
| // releases outside of the button, so we need to watch all the way | ||
| // from the top. | ||
| document.addEventListener("mouseup", this.handleMouseUp); | ||
| this.setTimeout(this.handleContinuousChange, NumericInput.CONTINUOUS_CHANGE_DELAY); |
There was a problem hiding this comment.
nice work with this.setTimeout 👍
though this is kinda the ideal use case for setInterval.
There was a problem hiding this comment.
Yeah, I thought I could handle both the initial delay and subsequent interval with setTimeout and a single handler function, but if it makes things clearer, I can rewrite the second case with setInterval. Looks like it's not defined in AbstractComponent though, so we'd have to use the global version.
| const nextValue = this.incrementValue(delta); | ||
| this.invokeValueCallback(nextValue, this.props.onButtonClick); | ||
| if (continuous) { |
There was a problem hiding this comment.
since this continuous flag is purely additive, would you kindly write it as a separate method and call it explicitly in the two handlers that use it?
| @@ -372,10 +424,19 @@ export class NumericInput extends AbstractPureComponent<HTMLInputProps & INumeri | |||
| this.setState({ isButtonGroupFocused: false }); | |||
| }; | |||
|
|
|||
| private handleButtonKeyDown = (e: React.KeyboardEvent<HTMLInputElement>, direction: IncrementDirection) => { | |||
There was a problem hiding this comment.
private updateDelta ? then you can reuse it everywhere else that this.delta... line appears.
There was a problem hiding this comment.
Not sure I see the advantage over just setting the variable directly; is it a convention to wrap private member access in a function? Anyway, done.
There was a problem hiding this comment.
if you make my proposed change in updateDelta then this method is not necessary, you can just use updateDelta directly.
| @@ -167,11 +167,15 @@ export class NumericInput extends AbstractPureComponent<HTMLInputProps & INumeri | |||
| */ | |||
| private static FLOATING_POINT_NUMBER_CHARACTER_REGEX = /^[Ee0-9\+\-\.]$/; | |||
|
|
|||
| private static CONTINUOUS_CHANGE_DELAY = 300; | |||
| private static CONTINUOUS_CHANGE_INTERVAL = 50; | |||
There was a problem hiding this comment.
can we slow this down? how about 100.
There was a problem hiding this comment.
Hm, that feels almost too slow to me. I tried matching the native HTML input (example, I used Firefox for reference), but I realize now that the exact behavior of that might also be OS-dependent. Of course, since NumericInput is not a native element, there is no obligation to imitate it exactly. That being the case, maybe we could go all-out with a gradual ramp up in speed? :-)
Edit: on second thought, that's what the shift key is for (can be toggled during mouse down as well), so input precision should be the goal here; I suppose you're right!
* Reduce continuous change interval to 100ms * Use setInterval() for the continuous change handler * Add and use updateDelta() instead of setting this.delta directly * Add and use startContinousChange() and stopContinousChange()
| @@ -594,4 +668,8 @@ export class NumericInput extends AbstractPureComponent<HTMLInputProps & INumeri | |||
| const scaleFactor = Math.pow(10, this.state.stepMaxPrecision); | |||
| return Math.round(value * scaleFactor) / scaleFactor; | |||
| } | |||
|
|
|||
| private updateDelta(value: number) { | |||
| this.delta = value; | |||
There was a problem hiding this comment.
🙅♂️ i meant that the entire logic should be here, that's the part that shouldn't be repeated:
// needs event arg
this.delta = this.getIncrementDelta(direction, e.shiftKey, e.altKey);
return this.delta;| // releases outside of the button, so we need to watch all the way | ||
| // from the top. | ||
| document.addEventListener("mouseup", this.handleMouseUp); | ||
| this.setTimeout(() => { |
There was a problem hiding this comment.
why wrap this in a timeout? if it's necessary, leave a code comment explaining why.
seems like just setting the interval directly should do it.
There was a problem hiding this comment.
There's an initial delay that's slightly longer (currently 300ms) than the regular interval that follows (currently 100ms); it's so that users don't trigger the continuous increment by accident. AFAIK it's not possible to change the interval of an existing setInterval() call. Does that make sense?
There was a problem hiding this comment.
ohhh yeah i see that. def leave a comment in the code.
| @@ -372,10 +424,19 @@ export class NumericInput extends AbstractPureComponent<HTMLInputProps & INumeri | |||
| this.setState({ isButtonGroupFocused: false }); | |||
| }; | |||
|
|
|||
| private handleButtonKeyDown = (e: React.KeyboardEvent<HTMLInputElement>, direction: IncrementDirection) => { | |||
There was a problem hiding this comment.
if you make my proposed change in updateDelta then this method is not necessary, you can just use updateDelta directly.
* Add a comment about wrapping setInterval() in setTimeout() * updateDelta() computes the delta itself using direction and event * Replace getIncrementDelta() calls with updateDelta() * Remove handleButtonKeyDown()
|
|
||
| private updateDelta( | ||
| direction: IncrementDirection, | ||
| e: React.MouseEvent<HTMLInputElement> | React.KeyboardEvent<HTMLInputElement>, |
There was a problem hiding this comment.
React.SyntheticEvent instead of union
There was a problem hiding this comment.
Tried that, but had to settle for the union because shiftKey and altKey aren't defined on SyntheticEvent.
| const nextValue = this.incrementValue(delta); | ||
| this.invokeValueCallback(nextValue, this.props.onButtonClick); | ||
| }; | ||
|
|
||
| private handleMouseUp = () => { | ||
| this.delta = 0; |
There was a problem hiding this comment.
this line would make more sense inside this.stopContinuousChange(), for completeness
| private handleMouseUp = () => { | ||
| this.delta = 0; | ||
| this.stopContinuousChange(); | ||
| document.removeEventListener("mouseup", this.handleMouseUp); |
There was a problem hiding this comment.
this also belongs inside this.stopContinuousChange(), as its counterpart appears in startContinuousChange
| const nextValue = this.incrementValue(delta); | ||
| this.invokeValueCallback(nextValue, this.props.onButtonClick); | ||
| }; | ||
|
|
||
| private handleMouseUp = () => { |
There was a problem hiding this comment.
after my comments below, can remove this function and replace references with onMouseUp={this.stopContinousChange}
| this.updateDelta(direction, e); | ||
| // respond explicitly on key *up*, because onKeyDown triggers multiple | ||
| // times and doesn't always receive modifier-key flags, leading to an | ||
| // unintuitive/out-of-control incrementing experience. |
There was a problem hiding this comment.
this comment does not make sense to me. what is this out-of-control experience?
would we be hurt down the line if this comment were removed?
There was a problem hiding this comment.
I didn't write this comment, but moved it from renderButton because it didn't make sense being there anymore (diff). It relates to the keyboard events we watch for when the ⌃ and ⌄ buttons have focus. In this case, space and enter act as if the user had clicked those buttons. If we listened on key down, then holding down space/enter would generate multiple events (that would actually be the expected behavior in my opinion), whereas key up is only generated once. I haven't tested the claim about not receiving modifier keys, but if that's what's stopping us from using onKeyDown then we should keep the comment.
* Remove handleMouseUp() in favor of stopContinuousChange() * Reset delta in stopContinuousChange()
More refactoringPreview: documentation | landing | table |
|
@reiv this is so awesome!! thank you! |
Fixes #2146
Changes proposed in this pull request:
This PR adds the ability to change the value of a numeric input by keeping the increment/decrement button pressed.
Reviewers should focus on:
Screenshot