fix: respect max and min in NumberInput when using arrows

[rudolpho1]

The arrow keys in the NumberInput allow users to step below the min and above the max value. This issue is fixed here.

Changelog

Changed

• Change the NumberInput to respect min and max values when using arrow keys

Testing / Reviewing

In order to reproduce the issue do the following:

• Create a NumberInput with min 0, max 100 and step 10

• manually enter the value 5 into the NumberInput

• click the arrow down.

actual behavior: the value of the NumberInput changes to -5

expected behavior: the value of the NumberInput changes to 0

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit 10d37cf

https://deploy-preview-4563--the-carbon-components.netlify.com

[netlify]

Deploy preview for carbon-elements ready!

Built with commit 10d37cf

https://deploy-preview-4563--carbon-elements.netlify.com

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit 10d37cf

https://deploy-preview-4563--carbon-components-react.netlify.com

[netlify]

Deploy preview for carbon-components-react failed.

Built with commit f0e41c5

https://app.netlify.com/sites/carbon-components-react/deploys/5de9795759fad60009964190

[netlify]

Deploy preview for carbon-elements ready!

Built with commit f0e41c5

https://deploy-preview-4563--carbon-elements.netlify.com

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit f0e41c5

https://deploy-preview-4563--the-carbon-components.netlify.com

[abbeyhrt]

Hi @rudolpho1! Thank you for taking the time to contribute. We are already addressing the issue with min/max not being respected as a part of an overhaul of NumberInput, here's the related issue: #2477, and we're taking a different approach UX-wise to address this problem. In the future, before creating a PR, could you open an issue with the problem you ran into? It would be really helpful for us so we can confirm the problem and make sure that no duplicate work is being done 👍

[asudoh]

Hi 👋 thank you for jumping in! Could you please fix the logic at

carbon/packages/react/src/components/NumberInput/NumberInput.js

Lines 221 to 222 in a5f960e

	? (min !== undefined && value > min) || min === undefined
	: (max !== undefined && value < max) || max === undefined;

rather than adding logic? Specifically, we need to ensure that the event handler won't run if the value shouldn't be changed.

[abbeyhrt]

@rudolpho1 you can ignore my previous comment! I interpreted the arrows as the arrow keys, not the increment and decrement buttons.

[rudolpho1]

@asudoh changing the conditional instead of the logic would mean that if the current value is 5 and you hit the decrement button the value would still be 5. But my expectation would be that it changes to the lowest possible value i.e. 0. With my logic, it is correct that the change handler fires, because the value does in fact change. If the current value is already the min (or max) value, then the conditional would already be false and nothing would happen and no event would be fired. So, I think the behaviour is correct this way. What do you think?

[asudoh]

OK makes sense - Thank you for your additional explanation @rudolpho1!

[emyarod]

looks good to me, would you mind adding/refactoring the existing tests for these scenarios (trying to increment past max or decrement below min)?

[emyarod]

this looks good to me but I just requested some testing for core component features since this seems like a common scenario