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.

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

Jump to bottom

Improve NumericInput and AbstractButton internals #4201

Merged
merged 6 commits into from
Jul 7, 2020
Merged

Improve NumericInput and AbstractButton internals #4201

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a20d2a
Improve NumericInput and AbstractButton internals
ejose19 Jun 30, 2020
b205b1b
Update packages/core/src/components/button/abstractButton.tsx
ejose19 Jul 7, 2020
cc63e71
refactor improvement on numericInput
ejose19 Jul 7, 2020
974a50a
change static property to public and adjust tests
ejose19 Jul 7, 2020
2c9762c
Update abstractButton.tsx
ejose19 Jul 7, 2020
ed168a6
remove instance access from numericInput
ejose19 Jul 7, 2020
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
14 changes: 14 additions & 0 deletions packages/core/src/components/button/abstractButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,20 @@ export abstract class AbstractButton<H extends React.HTMLAttributes<HTMLElement>

private currentKeyDown: number = null;

// A disabled element cannot be active, but there are cases where
// this situation can happen (NumericInput buttons), hence the need
// to check if disabled prop has been passed and change isActive
// accordingly
public static getDerivedStateFromProps(props: IButtonProps, state: IButtonState) {
if (state.isActive && props.disabled) {
return {
isActive: false,
};
}

return null;
}

public abstract render(): JSX.Element;

protected getCommonButtonProps() {
Expand Down
15 changes: 10 additions & 5 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 @@ -36,7 +36,7 @@ import {
import * as Errors from "../../common/errors";

import { ButtonGroup } from "../button/buttonGroup";
import { Button } from "../button/buttons";
import { AnchorButton } from "../button/buttons";
import { ControlGroup } from "./controlGroup";
import { InputGroup } from "./inputGroup";
import {
Expand Down Expand Up @@ -151,7 +151,12 @@ export interface INumericInputProps extends IIntentProps, IProps {
onButtonClick?(valueAsNumber: number, valueAsString: string): void;

/** The callback invoked when the value changes due to typing, arrow keys, or button clicks. */
onValueChange?(valueAsNumber: number, valueAsString: string, inputElement: HTMLInputElement | null): void;
onValueChange?(
valueAsNumber: number,
valueAsString: string,
inputElement: HTMLInputElement | null,
instance: NumericInput,
ejose19 marked this conversation as resolved.
Show resolved Hide resolved
): void;
}

export interface INumericInputState {
Expand Down Expand Up @@ -295,7 +300,7 @@ export class NumericInput extends AbstractPureComponent2<HTMLInputProps & INumer

if (!didControlledValueChange && this.state.value !== prevState.value) {
const { value: valueAsString } = this.state;
this.props.onValueChange?.(+valueAsString, valueAsString, this.inputElement);
this.props.onValueChange?.(+valueAsString, valueAsString, this.inputElement, this);
}
}

Expand Down Expand Up @@ -333,13 +338,13 @@ export class NumericInput extends AbstractPureComponent2<HTMLInputProps & INumer
const disabled = this.props.disabled || this.props.readOnly;
return (
<ButtonGroup className={Classes.FIXED} key="button-group" vertical={true}>
<Button
<AnchorButton
disabled={disabled || (value !== "" && +value >= max)}
icon="chevron-up"
intent={intent}
{...this.incrementButtonHandlers}
/>
<Button
<AnchorButton
disabled={disabled || (value !== "" && +value <= min)}
icon="chevron-down"
intent={intent}
Expand Down
84 changes: 44 additions & 40 deletions packages/core/test/controls/numericInputTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { spy } from "sinon";
import { dispatchMouseEvent, expectPropValidationError } from "@blueprintjs/test-commons";

import {
Button,
AnchorButton,
ButtonGroup,
ControlGroup,
HTMLInputProps,
Expand Down Expand Up @@ -87,7 +87,7 @@ describe("<NumericInput>", () => {
it("increments the value from 0 if the field is empty", () => {
const component = mount(<NumericInput />);

const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown");

const value = component.state().value;
Expand Down Expand Up @@ -197,20 +197,21 @@ describe("<NumericInput>", () => {
const onValueChangeSpy = spy();
const component = mount(<NumericInput onValueChange={onValueChangeSpy} />);

const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown");
dispatchMouseEvent(document, "mouseup");

const inputElement = component.find("input").first().getDOMNode();
expect(onValueChangeSpy.calledOnceWithExactly(1, "1", inputElement)).to.be.true;
const instance = component.instance();
expect(onValueChangeSpy.calledOnceWithExactly(1, "1", inputElement, instance)).to.be.true;
});

it("fires onButtonClick with the number value and the string value when either button is pressed", () => {
const onButtonClickSpy = spy();
const component = mount(<NumericInput onButtonClick={onButtonClickSpy} />);

const incrementButton = component.find(Button).first();
const decrementButton = component.find(Button).last();
const incrementButton = component.find(AnchorButton).first();
const decrementButton = component.find(AnchorButton).last();

// incrementing from 0
incrementButton.simulate("mousedown");
Expand Down Expand Up @@ -475,12 +476,12 @@ describe("<NumericInput>", () => {
// Enable these tests once we have a solution for testing Button onKeyUp callbacks (see PR #561)
describe("Keyboard interactions on buttons (with Space key)", () => {
const simulateIncrement = (component: ReactWrapper<any>, mockEvent: IMockEvent = {}) => {
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("keydown", addKeyCode(mockEvent, Keys.SPACE));
};

const simulateDecrement = (component: ReactWrapper<any>, mockEvent: IMockEvent = {}) => {
const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("keydown", addKeyCode(mockEvent, Keys.SPACE));
};

Expand All @@ -489,12 +490,12 @@ describe("<NumericInput>", () => {

describe("Keyboard interactions on buttons (with Enter key)", () => {
const simulateIncrement = (component: ReactWrapper<any>, mockEvent?: IMockEvent) => {
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("keydown", addKeyCode(mockEvent, Keys.ENTER));
};

const simulateDecrement = (component: ReactWrapper<any>, mockEvent?: IMockEvent) => {
const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("keydown", addKeyCode(mockEvent, Keys.ENTER));
};

Expand All @@ -503,12 +504,12 @@ describe("<NumericInput>", () => {

describe("Mouse interactions", () => {
const simulateIncrement = (component: ReactWrapper<any>, mockEvent?: IMockEvent) => {
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown", mockEvent);
};

const simulateDecrement = (component: ReactWrapper<any>, mockEvent?: IMockEvent) => {
const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("mousedown", mockEvent);
};

Expand All @@ -520,7 +521,7 @@ describe("<NumericInput>", () => {
it("enforces no minimum bound", () => {
const component = mount(<NumericInput />);

const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("mousedown", { shiftKey: true });
decrementButton.simulate("mousedown", { shiftKey: true });

Expand All @@ -531,7 +532,7 @@ describe("<NumericInput>", () => {
it("enforces no maximum bound", () => {
const component = mount(<NumericInput />);

const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown", { shiftKey: true });
incrementButton.simulate("mousedown", { shiftKey: true });

Expand Down Expand Up @@ -574,7 +575,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput min={MIN_VALUE} />);

// try to decrement by 1
const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("mousedown");

const newValue = component.state().value;
Expand All @@ -586,7 +587,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput min={MIN_VALUE} />);

// try to decrement by 1
const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("mousedown");

const newValue = component.state().value;
Expand All @@ -598,7 +599,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput min={MIN_VALUE} />);

// try to decrement by 0.1
const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("mousedown", { altKey: true });

const newValue = component.state().value;
Expand All @@ -610,7 +611,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput min={MIN_VALUE} />);

// try to decrement by 10
const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("mousedown", { shiftKey: true });

const newValue = component.state().value;
Expand All @@ -627,7 +628,8 @@ describe("<NumericInput>", () => {
expect(newValue).to.equal("0");

const inputElement = component.find("input").first().getDOMNode();
expect(onValueChangeSpy.calledOnceWithExactly(0, "0", inputElement)).to.be.true;
const instance = component.instance();
expect(onValueChangeSpy.calledOnceWithExactly(0, "0", inputElement, instance)).to.be.true;
});

it("does not fire onValueChange if nextProps.min < value", () => {
Expand All @@ -648,7 +650,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput max={MAX_VALUE} />);

// try to increment by 1
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown");

const newValue = component.state().value;
Expand All @@ -660,7 +662,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput max={MAX_VALUE} />);

// try to increment in by 1
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown");

const newValue = component.state().value;
Expand All @@ -672,7 +674,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput max={MAX_VALUE} />);

// try to increment by 0.1
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown", { altKey: true });

const newValue = component.state().value;
Expand All @@ -684,7 +686,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput max={MAX_VALUE} />);

// try to increment by 10
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown", { shiftKey: true });

const newValue = component.state().value;
Expand All @@ -701,7 +703,8 @@ describe("<NumericInput>", () => {
expect(newValue).to.equal("0");

const inputElement = component.find("input").first().getDOMNode();
expect(onValueChangeSpy.calledOnceWithExactly(0, "0", inputElement)).to.be.true;
const instance = component.instance();
expect(onValueChangeSpy.calledOnceWithExactly(0, "0", inputElement, instance)).to.be.true;
});

it("does not fire onValueChange if nextProps.max > value", () => {
Expand All @@ -722,7 +725,7 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput min={2} max={2} onValueChange={onValueChangeSpy} />);
// repeated interactions, no change in state
component
.find(Button)
.find(AnchorButton)
.first()
.simulate("mousedown")
.simulate("mousedown")
Expand All @@ -732,7 +735,8 @@ describe("<NumericInput>", () => {
expect(component.state().value).to.equal("2");

const inputElement = component.find("input").first().getDOMNode();
expect(onValueChangeSpy.calledOnceWithExactly(2, "2", inputElement)).to.be.true;
const instance = component.instance();
expect(onValueChangeSpy.calledOnceWithExactly(2, "2", inputElement, instance)).to.be.true;
});
});

Expand Down Expand Up @@ -835,7 +839,7 @@ describe("<NumericInput>", () => {
const value = component.state().value;
expect(value).to.equal("<invalid>");

const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();
incrementButton.simulate("mousedown");

const newValue = component.state().value;
Expand All @@ -848,7 +852,7 @@ describe("<NumericInput>", () => {
const value = component.state().value;
expect(value).to.equal("<invalid>");

const decrementButton = component.find(Button).last();
const decrementButton = component.find(AnchorButton).last();
decrementButton.simulate("mousedown");

const newValue = component.state().value;
Expand All @@ -869,8 +873,8 @@ describe("<NumericInput>", () => {
it("disables the increment button when the value is greater than or equal to max", () => {
const component = mount(<NumericInput value={100} max={100} />);

const decrementButton = component.find(Button).last();
const incrementButton = component.find(Button).first();
const decrementButton = component.find(AnchorButton).last();
const incrementButton = component.find(AnchorButton).first();

expect(decrementButton.props().disabled).to.be.false;
expect(incrementButton.props().disabled).to.be.true;
Expand All @@ -879,8 +883,8 @@ describe("<NumericInput>", () => {
it("disables the decrement button when the value is less than or equal to min", () => {
const component = mount(<NumericInput value={-10} min={-10} />);

const decrementButton = component.find(Button).last();
const incrementButton = component.find(Button).first();
const decrementButton = component.find(AnchorButton).last();
const incrementButton = component.find(AnchorButton).first();

expect(decrementButton.props().disabled).to.be.true;
expect(incrementButton.props().disabled).to.be.false;
Expand All @@ -890,8 +894,8 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput disabled={true} />);

const inputGroup = component.find(InputGroup);
const decrementButton = component.find(Button).last();
const incrementButton = component.find(Button).first();
const decrementButton = component.find(AnchorButton).last();
const incrementButton = component.find(AnchorButton).first();

expect(inputGroup.props().disabled).to.be.true;
expect(decrementButton.props().disabled).to.be.true;
Expand All @@ -902,8 +906,8 @@ describe("<NumericInput>", () => {
const component = mount(<NumericInput readOnly={true} />);

const inputGroup = component.find(InputGroup);
const decrementButton = component.find(Button).last();
const incrementButton = component.find(Button).first();
const decrementButton = component.find(AnchorButton).last();
const incrementButton = component.find(AnchorButton).first();

expect(inputGroup.props().readOnly).to.be.true;
expect(decrementButton.props().disabled).to.be.true;
Expand All @@ -926,8 +930,8 @@ describe("<NumericInput>", () => {
});

it("shows right element if provided", () => {
const component = mount(<NumericInput rightElement={<Button />} />);
expect(component.find(InputGroup).find(Button)).to.exist;
const component = mount(<NumericInput rightElement={<AnchorButton />} />);
expect(component.find(InputGroup).find(AnchorButton)).to.exist;
});

it("passed decimal value should be rounded by stepSize", () => {
Expand All @@ -942,7 +946,7 @@ describe("<NumericInput>", () => {

it("changes max precision of displayed value to that of the smallest step size defined", () => {
const component = mount(<NumericInput majorStepSize={1} stepSize={0.1} minorStepSize={0.001} />);
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();

incrementButton.simulate("mousedown");
expect(component.find("input").prop("value")).to.equal("0.1");
Expand All @@ -961,7 +965,7 @@ describe("<NumericInput>", () => {

it("changes max precision appropriately when the [*]stepSize props change", () => {
const component = mount(<NumericInput majorStepSize={1} stepSize={0.1} minorStepSize={0.001} />);
const incrementButton = component.find(Button).first();
const incrementButton = component.find(AnchorButton).first();

// excess digits should truncate to max precision
component.setState({ value: "0.0001" });
Expand Down