Fix useNumberFieldStately dep on aria #2918
Conversation
|
Build successful! 🎉 |
| useLayoutEffect(() => { | ||
| let [prevValue, setPrevValue] = useState(numberValue); | ||
| let [prevLocale, setPrevLocale] = useState(locale); | ||
| let [prevFormatOptions, setPrevFormatOptions] = useState(formatOptions); |
There was a problem hiding this comment.
These should be refs, not state. Setting them should not trigger a re-render. You could potentially also combine them into a single ref, that's an object containing them all?
There was a problem hiding this comment.
if they haven't changed, then setting then won't cause a re-render
we only get in here if one of them has changed, and we already call setState on the main one we're interested in
the setStates will get aggregated and result in only one new render
Note, this is the way that the react docs say to implement it as well https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops
There was a problem hiding this comment.
but I still don't get why it needs to be a state and not a ref? ref seems better for all things that don't need to cause a render.
There was a problem hiding this comment.
yeah, i'm not sure why they implement it with state, I've moved it over anyways. they should have the same behavior
|
Build successful! 🎉 |
|
Build successful! 🎉 |
…eact-spectrum into fix-numberfield-state-dependency
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
Closes
Make use of https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops
✅ Pull Request Checklist:
📝 Test Instructions:
Sanity check NumberField stories
🧢 Your Project: