Fix issues with SpinButton revealed by eslint-plugin-react-hooks #15431
Conversation
|
Thanks for submitting this change, but due to work currently in progress to prepare |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
|
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 34c4ff0:
|
Asset size changes
Baseline commit: e295779659b7466bd3e7e15bbc975cccb648549f (build) |
There was a problem hiding this comment.
Please wait on merging this until I can take a look again--I remember from the previous version of this that I had quite a bit of feedback, but I need to get some other things with v8 done ASAP and don't have time right away to review this in detail, sorry. :(
|
This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI! |
# Conflicts: # packages/react-examples/src/react/__snapshots__/SpinButton.Stateful.Example.tsx.shot # packages/react-internal/etc/react-internal.api.md # packages/react-internal/src/components/SpinButton/SpinButton.base.tsx # packages/react-internal/src/components/SpinButton/SpinButton.types.ts
…eat/react-internal-spinButton-onChange
…eat/react-internal-spinButton-onChange
…eat/react-internal-spinButton-onChange
|
Hi @czearing, I wanted to check if you are still planning to go forward with this PR? It looks like it's waiting for review from @ecraig12345. |
packages/react-internal/src/components/SpinButton/SpinButton.types.ts
Outdated
Show resolved
Hide resolved
ecraig12345
changed the title
|
After thinking about this and reading back through the great (and long) discussion on #5326, I think we should go ahead with the functionality fixes in this PR but revert the onChange addition for now. (But definitely get it done before the v8 release still!) Correctly implementing onChange has a lot of nuances which weren't covered here: mainly that we should only call it on "commit" rather than on every keystroke, plus lots of edge cases. See https://hackmd.io/cLn40UjfT_KOM6r4yi2CUQ for a full proposed spec. If the proposal seems good, either I can work on it (hopefully this week) or @czearing you can give it a try if you have time. Based on all that, I pushed some updates doing the following:
At this point the functionality issues should be fixed and I'm going to approve, but @dzearing should probably take a look as well so I'm not reviewing (partially) my own code. 😆 |
|
I should also have said, thank you so much @czearing for all your work on this and other changes, and SO sorry for not reviewing sooner!! |
|
In controlledValue mode, |
|
@kazuki The controlled SpinButton isn't working quite properly yet. I'll be fixing that in another PR this week. |
|
🎉 Handy links: |
Pull request checklist
$ yarn changeDescription of changes
1. Updating react-internalSpinButtonto handle onChange.2. Resolving issues revealed by eslint-plugin-react-hooks. #14480
3. Updating react-internalSpinButton.stateful.example.tsxto show the usage of onChange.4. Adding a forwardRef to
SpinButton.Focus areas to test
SpinButton