Skip to content
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

[core] feat(InputGroup): new prop onValueChange #6290

Merged
merged 5 commits into from
Jul 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/core/src/components/forms/inputGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@ export interface InputGroupProps
/** Whether this input should use large styles. */
large?: boolean;

/**
* Callback invoked when the input value changes, typically via keyboard interactions.
*
* Using this prop instead of `onChange` can help avoid common bugs in React 16 related to Event Pooling
* where developers forget to save the text value from a change event or call `event.persist()`.
*
* @see https://legacy.reactjs.org/docs/legacy-event-pooling.html
*/
onValueChange?(value: string, targetElement: HTMLInputElement | null): void;

/** Whether this input should use small styles. */
small?: boolean;

Expand All @@ -66,6 +76,8 @@ export interface InputGroupState {
rightElementWidth?: number;
}

const NON_HTML_PROPS: Array<keyof InputGroupProps> = ["onValueChange"];

/**
* Input group component.
*
Expand Down Expand Up @@ -120,8 +132,9 @@ export class InputGroup extends AbstractPureComponent<InputGroupProps, InputGrou
};
const inputProps = {
type: "text",
...removeNonHTMLProps(this.props),
...removeNonHTMLProps(this.props, NON_HTML_PROPS, true),
className: classNames(Classes.INPUT, inputClassName),
onChange: this.handleInputChange,
style,
};
const inputElement = asyncControl ? (
Expand Down Expand Up @@ -156,6 +169,12 @@ export class InputGroup extends AbstractPureComponent<InputGroupProps, InputGrou
}
}

private handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => {
const value = event.target.value;
this.props.onChange?.(event);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if it'd be reasonable to ban native onChange usage, let alone make such a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good.
imo the onValueChange signature is also an ergonomic improvement, so i anticipate it'll still have usage

this.props.onValueChange?.(value, event.target);
};

private maybeRenderLeftElement() {
const { leftElement, leftIcon } = this.props;

Expand Down
5 changes: 2 additions & 3 deletions packages/core/src/components/forms/numericInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -456,12 +456,12 @@ export class NumericInput extends AbstractPureComponent<HTMLInputProps & Numeric
leftIcon={this.props.leftIcon}
onFocus={this.handleInputFocus}
onBlur={this.handleInputBlur}
onChange={this.handleInputChange}
onCompositionEnd={this.handleCompositionEnd}
onCompositionUpdate={this.handleCompositionUpdate}
onKeyDown={this.handleInputKeyDown}
onKeyPress={this.handleInputKeyPress}
onPaste={this.handleInputPaste}
onValueChange={this.handleInputChange}
rightElement={this.props.rightElement}
small={this.props.small}
value={this.state.value}
Expand Down Expand Up @@ -616,8 +616,7 @@ export class NumericInput extends AbstractPureComponent<HTMLInputProps & Numeric
this.props.onPaste?.(e);
};

private handleInputChange = (e: React.FormEvent) => {
const { value } = e.target as HTMLInputElement;
private handleInputChange = (value: string) => {
let nextValue = value;
if (this.props.allowNumericCharactersOnly && this.didPasteEventJustOccur) {
this.didPasteEventJustOccur = false;
Expand Down
14 changes: 14 additions & 0 deletions packages/core/test/controls/inputGroupTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,18 @@ describe("<InputGroup>", () => {
// value should not change because our change handler prevents it from being longer than characters
assert.strictEqual(input.prop("value"), "abc");
});

it("supports the onValueChange callback", () => {
const initialValue = "value";
const newValue = "new-value";
const handleValueChange = spy();
const inputGroup = mount(<InputGroup value={initialValue} onValueChange={handleValueChange} />);
assert.strictEqual(inputGroup.find("input").prop("value"), initialValue);

inputGroup
.find("input")
.simulate("change", { currentTarget: { value: newValue }, target: { value: newValue } });
assert.isTrue(handleValueChange.calledOnce, "onValueChange not called");
assert.isTrue(handleValueChange.calledWithMatch(newValue), `onValueChange not called with '${newValue}'`);
});
});
7 changes: 2 additions & 5 deletions packages/docs-app/src/components/icons.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ export class Icons extends React.PureComponent<IconsProps, IconsState> {
className={Classes.FILL}
large={true}
leftIcon="search"
onValueChange={this.handleFilterChange}
placeholder="Search for icons..."
onChange={this.handleFilterChange}
type="search"
value={this.state.filter}
/>
Expand Down Expand Up @@ -100,10 +100,7 @@ export class Icons extends React.PureComponent<IconsProps, IconsState> {
return icons.filter(icon => iconFilter(this.state.filter, icon));
}

private handleFilterChange = (e: React.SyntheticEvent<HTMLInputElement>) => {
const filter = (e.target as HTMLInputElement).value;
this.setState({ filter });
};
private handleFilterChange = (filter: string) => this.setState({ filter });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

}

function isIconFiltered(query: string, icon: Icon) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,5 @@ export class BreadcrumbsExample extends React.PureComponent<ExampleProps, Breadc

const BreadcrumbInput: React.FC<BreadcrumbProps & { defaultValue: string | undefined }> = props => {
const [text, setText] = React.useState(props.defaultValue ?? "");
const handleChange = React.useCallback((event: React.FormEvent<HTMLInputElement>) => {
setText((event.target as HTMLInputElement).value);
}, []);
return <InputGroup placeholder="rename me" value={text} onChange={handleChange} />;
return <InputGroup placeholder="rename me" value={text} onValueChange={setText} />;
};
Original file line number Diff line number Diff line change
Expand Up @@ -48,24 +48,20 @@ export class FileInputExample extends React.PureComponent<ExampleProps, FileInpu
<>
<H5>Props</H5>
<FormGroup label="Text">
<InputGroup placeholder="Choose file..." onChange={this.handleTextChange} value={text} />
<InputGroup placeholder="Choose file..." onValueChange={this.handleTextChange} value={text} />
</FormGroup>
<FormGroup label="Button text">
<InputGroup placeholder="Browse" onChange={this.handleButtonTextChange} value={buttonText} />
<InputGroup placeholder="Browse" onValueChange={this.handleButtonTextChange} value={buttonText} />
</FormGroup>
<Switch label="Large" onChange={this.handleLargeChange} checked={large} />
<Switch label="Small" onChange={this.handleSmallChange} checked={small} />
</>
);
};

private handleTextChange = (e: React.ChangeEvent<HTMLInputElement>) => {
this.setState({ text: e.target.value });
};
private handleTextChange = (text: string) => this.setState({ text });

private handleButtonTextChange = (e: React.ChangeEvent<HTMLInputElement>) => {
this.setState({ buttonText: e.target.value });
};
private handleButtonTextChange = (buttonText: string) => this.setState({ buttonText });

private handleSmallChange = handleBooleanChange(small => this.setState({ small, ...(small && { large: false }) }));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export class TabsExample extends React.PureComponent<ExampleProps, TabsExampleSt
<Tab id="mb" title="Ember" panel={<EmberPanel />} panelClassName="ember-panel" />
<Tab id="bb" disabled={true} title="Backbone" panel={<BackbonePanel />} />
<Tabs.Expander />
<InputGroup className={Classes.FILL} type="text" placeholder="Search..." />
<InputGroup fill={true} type="text" placeholder="Search..." />
</Tabs>
</Example>
);
Expand Down