-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TextField: Call onChange when cleared #1102
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. Thanks for the careful explanation here, and for the fix!
That said: I am the least-expert person when it comes to the internal workings of our components and I'd love to get @JohnC-80's eyes on this before committing it just in case there is some special case where passing an empty string to onChange
kills a puppy or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But I think @JohnC-80's suggestion would make sense. And we could maybe even allow for the onClearField
-callback passed as a prop to still work.
Maybe pass in the input
-prop as a parameter if a developer has passed the callback to a react-final-form
-field. And if not; just default to resetting the value.
onClearField: typeof onClearField === 'function' ? () => onClearField(input) : () => input.onChange(''),
@JohnC-80, I think your suggestion breaks supplying an @rasmuswoelk's way of passing the So really, I think we'd have to change the API so that it's |
Yeah, I definitely don't like having to call The original approach that you have, calling |
You're right, @doytch my original suggestion wouldn't work anyways since if the app was using the We could apply the config last in |
…ents into textfield-clear
Makes sense, @JohnC-80. I've pushed up a change that just persists React's synthetic element and passes it along instead of using the pass-value shorthand. Also works 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can handle this. I had reserve that the input wouldn't contain the ''
value yet when this onChange was called... but if it works, it works!
Double-checked the API; apparently React guarantees the setState callback will be fired not just after the update is applied, but also after the component has re-rendered. |
That said, I went ahead and traced through the execution. The event Soooo, yeah. |
🤣 and 😭 |
Give this branch a go... In short, this changes the value of the input natively and then dispatches the change event, which catches the This has been the adopted approach in a few places, by a few people but I wouldn't want to do this all the time. In this branch, the clear functionality is handled as follows:
Previously, we were changing the state and attempting to dispatch within a callback. I believe this didn't work because React does a little bit of reducing/batching/deduping when it comes to change event handling. Since the state was updated, the field was re-rendered with the Additionally, the long-windedness of that whole |
That looks like it works well. Tested in RFF (Agreements), RF (Users), and uncontrolled inputs (SearchField). My initial squeemishness remains in place, but the amount of people deploying this workaround and underlying logic leads me to believe we're relatively safe with it. Want me to update this PR with that option or do you have a branch going that you want to use? Diff leads me to believe you may have some tests that you wrote already? |
Go ahead and update this PR.. I think the workspace I was using for the other might have been stale. I haven't added any tests. I assume existing tests for clearing a |
Ran into an interesting bug when fixing a separate bug today.
When you hit the clear icon (X) at the end of a TextField, React Final Form (and I believe Redux Form as well) don't actually get notified of this.
onClearField
is called, but that's not provided by either of the form libraries.Note how
onChange
is called inTextField.handleChange
. In handleClear, we use the pass-value way of calling onChange because otherwise we'd have to persist the event to have it available in the setState callback.