Improve NumericInput and AbstractButton internals

[ejose19]

Fixes #4200

Checklist

• Includes tests
• Update documentation (No changes to docs)

Changes proposed in this pull request:

• Add a check to each iteration of handleContinuousChange, since mouseup event won't fire if the mouse is released on the disabled element (after it reached min/max limit) current behaviour keeps running it indefinitely when using min/max props and releasing the mouseup after described event. Fixed using AnchorButton

• Allow accessing class instance during onValueChange callback, this is necessary as if doing any conditional logic to allow a value, one needs to update the internal value to keep it in sync with given value prop (see my comment on this issue for more details) Removed, another PR improves controlled mode

• Add componentDidUpdate getDerivedStateFromProps to AbstractButton, related to the 1st issue, its handleKeyUp is never fired if the button is disabled, so internal state is still "active" while the button is disabled (which is not correct) displaying inconsistent UI to the user when using along other components (For example NumericInput)

Reviewers should focus on:

Mentioned changes

Screenshot

N/A

[palantirtech]

Thanks for your interest in palantir/blueprint, @ejose19! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[adidahiya]

Allow accessing class instance during onValueChange callback, this is necessary as if doing any conditional logic to allow a value, one needs to update the internal value to keep it in sync with given value prop

I'm not yet convinced that this is necessary. What kind of conditional logic are you doing? Can we support that use case some other way instead of providing access to the component's internals? I don't want to increase the size of the public API this way.

---

Also, I think this whole issue might be easier solved by simply replacing the Button components rendered by NumericInput with AnchorButton components... they are able to receive events when disabled=true

[adidahiya]

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;
    }

[ejose19]

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

[adidahiya]

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

[ejose19]

@adidahiya Regarding the instance access, if there's any other way then great, but I would like to know how you would do that. Here's the issue: Controlled mode, my value is set to 1, and NumericInput sets its internal state to '1', now I only want integers in a range, so I add a conditional logic to onValueChange callback to apply the new value to my state, this theorically works good if the user is either using the buttons or tapping only numbers, but there are 2 cases: (pressing the letter 'e' is allowed, and pasting content into the field) first action is allowed and internal value is stored as '1e' but my state rejected it and it's 1, now they're not in sync and buttons won't display limits properly. On paste if text pasted is invalid (letters only), then internal state is VALUE_EMPTY and my state is still 1. So either we need to add a callback function like ("isValid") to determine if internal state should change, or as I'm doing in this patch, let me change the internal state manually after I do my checks.

[ejose19]

Applied both recommendations and I confirm they work (Using AnchorButton on numericInput and getDerivedStateFromProps on abstractButton)

[ejose19]

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

[adidahiya]

@ejose19 regarding controlled mode... check out the PR I just opened #4211... that should get you sorted. For this PR please just stick to fixing #4200. I'll merge my PR today and then you can test it.

[adidahiya]

thanks for testing my branch. I'll merge yours first so you don't have to deal with conflicts

[ejose19]

@adidahiya Since there's a side effect caused by using AnchorButton, should we use the original idea instead?

Add a check to each iteration of handleContinuousChange, since mouseup event won't fire if the mouse is released on the disabled element (after it reached min/max limit) current behaviour keeps running it indefinitely when using min/max props and releasing the mouseup after described event.

[adidahiya]

@ejose19 ok, yeah, I would be open to a change that switches to the original idea using Button instead