Skip to content

Commit

Permalink
[core] fix(NumericInput): do not trigger onValueChange if value is ch…
Browse files Browse the repository at this point in the history 
…anged via props (#3814)
  • Loading branch information
@adidahiya
adidahiya authored Nov 7, 2019
1 parent 8546bc0 commit 9fcca7a
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 54 deletions.
97 changes: 48 additions & 49 deletions packages/core/src/components/forms/numericInput.tsx
View file
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
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

1 comment on commit 9fcca7a

@blueprint-bot
Copy link

Choose a reason for hiding this comment

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

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

Previews: documentation | landing | table

Please sign in to comment.