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 1 commit
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
12 changes: 12 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 @@ -134,6 +134,16 @@ export abstract class AbstractButton<H extends React.HTMLAttributes<HTMLElement>
};
}

// A disabled element cannot be active, there're are cases where
ejose19 marked this conversation as resolved.
Show resolved Hide resolved
// this situation can happen (NumericInput buttons), hence the need
// to check if disabled prop has been passed and change isActive
// accordingly
public componentDidUpdate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be implemented inside static getDerivedStateFromProps instead:

    protected static getDerivedStateFromProps(props: IButtonProps, state: IButtonState) {
        if (state.isActive && props.disabled) {
            return {
                isActive: false,
            };
        }

        return null;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is better than componentDidUpdate then no issues, I will change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's better because it's a more natural place to manage state updates, it gets called before render. Check out the React lifecycle documentation to see what kinds of behavior is recommended in the various methods

if (this.state.isActive && this.props.disabled) {
this.setState({ isActive: false });
}
}

// we're casting as `any` to get around a somewhat opaque safeInvoke error
// that "Type argument candidate 'KeyboardEvent<T>' is not a valid type
// argument because it is not a supertype of candidate
Expand All @@ -149,6 +159,8 @@ export abstract class AbstractButton<H extends React.HTMLAttributes<HTMLElement>
this.props.onKeyDown?.(e);
};

// Keyup event won't fire on a disabled element, so componentDidUpdate
// will take care of those cases and set isActive to false
protected handleKeyUp = (e: React.KeyboardEvent<any>) => {
if (Keys.isKeyboardClick(e.which)) {
this.setState({ isActive: false });
Expand Down
23 changes: 21 additions & 2 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 @@ -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 @@ -422,7 +427,21 @@ export class NumericInput extends AbstractPureComponent2<HTMLInputProps & INumer
document.removeEventListener("mouseup", this.stopContinuousChange);
};

// If limit is reached and mouseup event occurs on top of the disabled button
// stopContinuousChange will never be fired, hence the need to check each
// iterarion of handleContinuousChange
private limitReachedOnContinuousChange = () => {
const min = this.props.min ?? -Infinity;
const max = this.props.max ?? Infinity;
if (Number(this.state.value) <= min || Number(this.state.value) >= max) {
this.stopContinuousChange();
return true;
}
return false;
};

private handleContinuousChange = () => {
if (this.limitReachedOnContinuousChange()) return;
const nextValue = this.incrementValue(this.delta);
this.props.onButtonClick?.(+nextValue, nextValue);
};
Expand Down
12 changes: 8 additions & 4 deletions packages/core/test/controls/numericInputTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ describe("<NumericInput>", () => {
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", () => {
Expand Down Expand Up @@ -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 Down Expand Up @@ -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 Down Expand Up @@ -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