[core] feat(InputGroup): new prop onValueChange
#6290
Conversation
| @@ -156,6 +164,12 @@ export class InputGroup extends AbstractPureComponent<InputGroupProps, InputGrou | |||
| } | |||
| } | |||
|
|
|||
| private handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => { | |||
| const value = event.currentTarget.value; | |||
| this.props.onChange?.(event); | |||
There was a problem hiding this comment.
I wasn't sure if it'd be reasonable to ban native onChange usage, let alone make such a breaking change.
There was a problem hiding this comment.
No, we won't ban it, and we can't make a breaking change like that right now. We can just update the docs and maybe create a lint rule.
There was a problem hiding this comment.
Event pooling is going away in React 17, so I think this will become less of a problem: https://legacy.reactjs.org/blog/2020/08/10/react-v17-rc.html#no-event-pooling
I added a note in the JSDoc for the new prop about why users might prefer this handler instead of onChange.
There was a problem hiding this comment.
sounds good.
imo the onValueChange signature is also an ergonomic improvement, so i anticipate it'll still have usage
adidahiya
changed the title
onValueChange
|
docs preview looks fine 👍🏽 |
| const filter = (e.target as HTMLInputElement).value; | ||
| this.setState({ filter }); | ||
| }; | ||
| private handleFilterChange = (filter: string) => this.setState({ filter }); |
Checklist
Changes proposed in this pull request:
Add a new
onValueChangecallback prop to<InputGroup>which behaves similarly to the same prop for<NumericInput>.Motivation: this helps prevent common potential footguns with React 16 Event Pooling where developers sometimes forget to save
event.target.valueas a variable or useevent.persist(), which can lead to null-pointer exceptions at runtime.Various uses of
<InputGroup onChange>within the Blueprint monorepo have also been updated to use the new prop to demonstrate its efficacy.Reviewers should focus on:
docs & tests for the new prop
Screenshot
N/A