Skip to content

[core] fix(NumericInput): do not trigger onValueChange if value is changed via props #3814

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 7, 2019
Merged

[core] fix(NumericInput): do not trigger onValueChange if value is changed via props #3814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f651b9b
[core] fix(NumericInput): do not trigger onValueChange if value is ch…
adidahiya Nov 4, 2019
0527c75
Fix tests
adidahiya Nov 7, 2019
7ee4afd
Merge remote-tracking branch 'origin/develop' into ad/fix-numeric-input
adidahiya Nov 7, 2019
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
97 changes: 48 additions & 49 deletions packages/core/src/components/forms/numericInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,14 @@ export interface INumericInputProps extends IIntentProps, IProps {
}

export interface INumericInputState {
prevMinProp?: number;
prevMaxProp?: number;
prevValueProp?: number | string;
shouldSelectAfterUpdate: boolean;
stepMaxPrecision: number;
value: string;
}

export interface INumericInputSnapshot {
value: string;
}

enum IncrementDirection {
DOWN = -1,
UP = +1,
Expand All @@ -185,11 +184,7 @@ const NON_HTML_PROPS: Array<keyof INumericInputProps> = [
type ButtonEventHandlers = Required<Pick<React.HTMLAttributes<Element>, "onKeyDown" | "onMouseDown">>;

@polyfill
export class NumericInput extends AbstractPureComponent2<
HTMLInputProps & INumericInputProps,
INumericInputState,
INumericInputSnapshot
> {
export class NumericInput extends AbstractPureComponent2<HTMLInputProps & INumericInputProps, INumericInputState> {
public static displayName = `${DISPLAYNAME_PREFIX}.NumericInput`;

public static VALUE_EMPTY = "";
Expand All @@ -208,10 +203,30 @@ export class NumericInput extends AbstractPureComponent2<
value: NumericInput.VALUE_EMPTY,
};

public static getDerivedStateFromProps(props: INumericInputProps) {
public static getDerivedStateFromProps(props: INumericInputProps, state: INumericInputState) {
const nextState = { prevMinProp: props.min, prevMaxProp: props.max, prevValueProp: props.value };

const didMinChange = props.min !== state.prevMinProp;
const didMaxChange = props.max !== state.prevMaxProp;
const didBoundsChange = didMinChange || didMaxChange;

const didValuePropChange = props.value !== state.prevValueProp;
const value = getValueOrEmptyValue(didValuePropChange ? props.value : state.value);

const sanitizedValue =
value !== NumericInput.VALUE_EMPTY
? NumericInput.getSanitizedValue(value, /* delta */ 0, props.min, props.max)
: NumericInput.VALUE_EMPTY;

const stepMaxPrecision = NumericInput.getStepMaxPrecision(props);

return { stepMaxPrecision };
// if a new min and max were provided that cause the existing value to fall
// outside of the new bounds, then clamp the value to the new valid range.
if (didBoundsChange && sanitizedValue !== state.value) {
return { ...nextState, stepMaxPrecision, value: sanitizedValue };
} else {
return { ...nextState, stepMaxPrecision, value };
}
}

private static CONTINUOUS_CHANGE_DELAY = 300;
Expand All @@ -227,6 +242,14 @@ export class NumericInput extends AbstractPureComponent2<
}
}

private static getSanitizedValue(value: string, stepMaxPrecision: number, min: number, max: number, delta = 0) {
if (!isValueNumeric(value)) {
return NumericInput.VALUE_EMPTY;
}
const nextValue = toMaxPrecision(parseFloat(value) + delta, stepMaxPrecision);
return clampValue(nextValue, min, max).toString();
}

public state: INumericInputState = {
shouldSelectAfterUpdate: false,
stepMaxPrecision: NumericInput.getStepMaxPrecision(this.props),
Expand All @@ -242,24 +265,6 @@ export class NumericInput extends AbstractPureComponent2<
private incrementButtonHandlers = this.getButtonEventHandlers(IncrementDirection.UP);
private decrementButtonHandlers = this.getButtonEventHandlers(IncrementDirection.DOWN);

public getSnapshotBeforeUpdate(prevProps: INumericInputProps): INumericInputSnapshot {
const didMinChange = prevProps.min !== this.props.min;
const didMaxChange = prevProps.max !== this.props.max;
const didBoundsChange = didMinChange || didMaxChange;

const baseValue = prevProps.value !== this.props.value ? this.props.value : this.state.value;
const value = getValueOrEmptyValue(baseValue);

const sanitizedValue =
value !== NumericInput.VALUE_EMPTY
? this.getSanitizedValue(value, /* delta */ 0, this.props.min, this.props.max)
: NumericInput.VALUE_EMPTY;

return {
value: didBoundsChange ? sanitizedValue : value,
};
}

public render() {
const { buttonPosition, className, fill, large } = this.props;
const containerClasses = classNames(Classes.NUMERIC_INPUT, { [Classes.LARGE]: large }, className);
Expand All @@ -273,19 +278,16 @@ export class NumericInput extends AbstractPureComponent2<
);
}

public componentDidUpdate(
prevProps: INumericInputProps,
prevState: INumericInputState,
snapshot: INumericInputSnapshot,
) {
super.componentDidUpdate(prevProps, prevState, snapshot);
public componentDidUpdate(prevProps: INumericInputProps, prevState: INumericInputState) {
super.componentDidUpdate(prevProps, prevState);
if (this.state.shouldSelectAfterUpdate) {
this.inputElement.setSelectionRange(0, this.state.value.length);
}

this.setState({ value: snapshot.value });
if (this.state.value !== snapshot.value) {
this.invokeValueCallback(snapshot.value, this.props.onValueChange);
const didControlledValueChange = this.props.value !== prevProps.value;

if (!didControlledValueChange && this.state.value !== prevState.value) {
this.invokeValueCallback(this.state.value, this.props.onValueChange);
}
}

Expand Down Expand Up @@ -421,9 +423,6 @@ export class NumericInput extends AbstractPureComponent2<
const { value } = e.target as HTMLInputElement;
const sanitizedValue = this.getSanitizedValue(value);
this.setState({ value: sanitizedValue });
if (value !== sanitizedValue) {
this.invokeValueCallback(sanitizedValue, this.props.onValueChange);
}
}

Utils.safeInvoke(this.props.onBlur, e);
Expand Down Expand Up @@ -487,7 +486,6 @@ export class NumericInput extends AbstractPureComponent2<
}

this.setState({ shouldSelectAfterUpdate: false, value: nextValue });
this.invokeValueCallback(nextValue, this.props.onValueChange);
};

private invokeValueCallback(value: string, callback: (valueAsNumber: number, valueAsString: string) => void) {
Expand All @@ -500,7 +498,6 @@ export class NumericInput extends AbstractPureComponent2<
const nextValue = this.getSanitizedValue(currValue, delta);

this.setState({ shouldSelectAfterUpdate: this.props.selectAllOnIncrement, value: nextValue });
this.invokeValueCallback(nextValue, this.props.onValueChange);

return nextValue;
}
Expand All @@ -517,12 +514,14 @@ export class NumericInput extends AbstractPureComponent2<
}
}

private getSanitizedValue(value: string, delta = 0, min = this.props.min, max = this.props.max) {
if (!isValueNumeric(value)) {
return NumericInput.VALUE_EMPTY;
}
const nextValue = toMaxPrecision(parseFloat(value) + delta, this.state.stepMaxPrecision);
return clampValue(nextValue, min, max).toString();
private getSanitizedValue(value: string, delta = 0) {
return NumericInput.getSanitizedValue(
value,
this.state.stepMaxPrecision,
this.props.min,
this.props.max,
delta,
);
}

private updateDelta(direction: IncrementDirection, e: React.MouseEvent | React.KeyboardEvent) {
Expand Down
19 changes: 14 additions & 5 deletions packages/core/test/controls/numericInputTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,19 +153,19 @@ describe("<NumericInput>", () => {

it("provides numeric value to onValueChange as a number and a string", () => {
const onValueChangeSpy = spy();
const component = mount(<NumericInput onValueChange={onValueChangeSpy} value={1} />);
const component = mount(<NumericInput onValueChange={onValueChangeSpy} />);

component.find("input").simulate("change");
component.setState({ value: "1" });

expect(onValueChangeSpy.calledOnce).to.be.true;
expect(onValueChangeSpy.calledWith(1, "1")).to.be.true;
});

it("provides non-numeric value to onValueChange as NaN and a string", () => {
const onValueChangeSpy = spy();
const component = mount(<NumericInput onValueChange={onValueChangeSpy} value={"non-numeric-value"} />);
const component = mount(<NumericInput onValueChange={onValueChangeSpy} />);

component.find("input").simulate("change");
component.setState({ value: "non-numeric-value" });

expect(onValueChangeSpy.calledOnce).to.be.true;
expect(onValueChangeSpy.calledWith(NaN, "non-numeric-value")).to.be.true;
Expand Down Expand Up @@ -714,7 +714,7 @@ describe("<NumericInput>", () => {
.simulate("mousedown")
.simulate("mousedown");
expect(component.state().value).to.equal("2");
expect(onValueChangeSpy.callCount).to.equal(5);
expect(onValueChangeSpy.callCount).to.equal(1);
expect(onValueChangeSpy.args[0]).to.deep.equal([2, "2"]);
});
});
Expand Down Expand Up @@ -839,6 +839,15 @@ describe("<NumericInput>", () => {
});
});

describe("Controlled mode", () => {
it("value prop updates do not trigger onValueChange", () => {
const onValueChangeSpy = spy();
const component = mount(<NumericInput min={0} value={0} max={1} onValueChange={onValueChangeSpy} />);
component.setProps({ value: 1 });
expect(onValueChangeSpy.notCalled).to.be.true;
});
});

describe("Other", () => {
it("disables the input field and buttons when disabled is true", () => {
const component = mount(<NumericInput disabled={true} />);
Expand Down