SpinButton: Add onChange #16137
Merged
SpinButton: Add onChange #16137
Conversation
This file contains 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
dzearing
approved these changes
Dec 4, 2020
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a177fff:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changes
Baseline commit: 3ddffa4fb783da403f11ad1fc1496bb727dd0585 (build) |
|
Hello! I'm experiencing an issue that looks like onChange is incorrectly passed to a wrapper div. Sorry for the random comment, but I thought I'd drop a link here since this is the relevant PR that added onChange functionality to SpinButton. Issue details: #18153 |
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.
Pull request checklist
$ yarn changeDescription of changes
Finally add an
onChangehandler to SpinButton, with lots of tests.One important step before this was improving handling of intermediate values: only store "committed"/validated values in
value, and add a new stateintermediateValue(instead oflastValidValueandvalueToValidate) to store intermediate values as the user is typing in the field. You can see most of this part alone in 1ef1a36.I also updated theEDIT: this caused problems with Checkbox (and possibly other things) and I actually ended up solving the issue that motivated this change another way.useControllableValuehook so that it only callsonChangeif the value actually changed. This appears to match React's behavior with input elements, and overall I'd guess it's more likely to match user expectations.Spec
This is the spec I followed for
onChangebehavior, based on the discussion on #5326 plus some updates to reflect actual implementation details and challenges (see #5326 (comment) for the original version).Intermediate vs. committed values
SpinButton can have two types of values: intermediate and committed.
A value is intermediate while the user is editing the text field.
The value is committed when any of the following happens:
The commit sequence consists of these steps:
onIncrement,onDecrement, oronValidateas appropriatesetValuereturned byuseControllableValue, which will:onChangeif providedintermediateValue(causing the committed value to be displayed)Determining and storing the displayed value
Where is the value stored?
valuereturned byuseControllableValue(this will beprops.valueif controlled, or it tracks state if uncontrolled)intermediateValuestateWhat is the initial value? (in order of precedence)
props.value(no validation)defaultValue(no validation)minWhat is the currently displayed value?
intermediateValueif set (while the user is editing text)valuereturned byuseControllableValue(this will beprops.valueif controlled)What is the value of
ISpinButton.value?valuereturned byuseControllableValue(this will beprops.valueif controlled)intermediateValuehereEdge cases while editing text:
props.valuewe should retainintermediateValue(remain in edit mode)props.value, we should clearintermediateValue(and exit edit mode without callingonChange)How events are handled
Editing text: Update
intermediateValueon each keystroke but do not callonChangeuntil commit.Blur or enter keypress (input field): Run the commit sequence starting with
onValidate(onChangewill be called only if the value changed).Arrow button click or spin:
intermediateValueis set), we have to allow the typed value to be committed before handling the increment/decrement:onBlurruns, triggering the commit sequence (includingonChange) and likely a value updateonIncrement/onDecrement(includingonChange)Arrow key press or spin
intermediateValueis set), we have to commit the typed value before handling the increment/decrement:onChange), which will likely cause a value updateonIncrement/onDecrement(includingonChange)Other design decisions
In some cases, the "correct" behavior was not entirely clear. Documenting some of those cases and the reasoning below.
props.valueorprops.defaultValue(this is certainly correct forvaluebut debatable fordefaultValue)onChangeis called for every spin, not only when spinning stops (no notion of intermediate values). We went with this because it's not uncommon that on every press, something in the app will react, like a font size going up and down.onChangeis not called for every keystroke when editing the text. We went with this because in this case, typing over the field seems more like hovering over options in a dropdown. If someone has a compelling need for notification of these intermediate values, we could consider adding something to handle that.ISpinButton.valueonly returns the committed value, not the intermediate typed value. If someone has a compelling need for access to these intermediate values, we could consider adding something to handle that.props.valueis provided, the component is no longer fully controllable using just the return values ofonIncrement/onDecrement/onValidate. This may be a breaking change for some people, though in most cases it could be mitigated by switching todefaultValue.