fix(NumberInput): Allow empty input for NumberInput with allowEmpty prop (#15985)

[hollyos]

Closes #15985
Closes #16016

There were multiple issues open for functionality in the NumberInput.

1. The input did not allow for an empty input, even with allowEmpty = true. Number() automatically assigns 0 to an empty string. A ternary was added to capture this instance and allow for the empty string when allowEmpty = true. It will still default to 0 when allowEmpty = false.

2. The input did not allow for negative input. I tested this in the live demo and Storybook. It works in both instances. This issue was likely due to the min value equaling 0. When min is updated to a negative value, the input allows for negatives.

   

Changelog

Changed

• allow empty inputs when allowEmpty = true

Testing / Reviewing

1. Start Storybook
2. visit NumberInput Playground on localhost:3000.
3. Ensure the following args are updated:

   prop	value	purpose
   allowEmpty	true	Ensures that an empty value can be used
   min	-200	Allows for negative values. The default of 0 causes an error to display.

4. With these params set, select then delete the input value. It should be an empty input.
5. Then enter a negative value greater than the min value. Should allow the value and not show an error.

[github-actions]

DCO Assistant Lite bot All contributors have signed the DCO.

[netlify]

✅ Deploy Preview for v11-carbon-react ready!

Name	Link
🔨 Latest commit	ab74bc3
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/661ec4f11cd5c1000733b8a3
😎 Deploy Preview	https://deploy-preview-16100--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[hollyos]

I have read the DCO document and I hereby sign the DCO.

[hollyos]

recheck

[tw15egan]

LGTM 👍 ✅ Thanks for contributing and the detailed explanations! Would updating the helper text to display the minimum or maximum or updating the example to a minimum of -100 help alleviate the original confusion?

[hollyos]

LGTM 👍 ✅ Thanks for contributing and the detailed explanations! Would updating the helper text to display the minimum or maximum or updating the example to a minimum of -100 help alleviate the original confusion?

Hi @tw15egan, yes, I'd imagine adding the context to the helper text would alleviate some of the confusion. Though, would this be a default? Does this then occur for every param that could throw an error?

In this instance, might just be worth updating the helper text in the implementation of the storybook demo first.

After which, it could be worth defining better what should be shipped in the design system library vs what should be added in the individual implementation without adding a bunch of cruft or overhead. Is it worth a different issue for planning that feature set?

[tw15egan]

@hollyos Yeah, that's a good point, I think updating the storybook is all that's needed. We can track that separately

[hollyos]

@tw15egan currently, the validity checker is only a bool response. There's no context as to the reason for the failure. Without adding a bunch of overhead, may just be worth updating invalidText="Number is not valid" to invalidText="Number is not valid, update knobs below" for the Playground implementation. Thoughts?

[2nikhiltom]

Hey @hollyos Thanks for your contribution, this looks good 🔥
I've set the minimum value in the storybook example to -100, I believe this should alleviate the original confusion

<NumberInput
     id="carbon-number"
     min={-100}
     max={100}
     value={50}
     label="NumberInput label"
     helperText="Optional helper text."
     invalidText="Number is not valid"
     {...args}
   />

[andreancardona]

@hollyos there are also some failing AVT tests, do you mind fixing those? Thank you!

outdated

[tay1orjones]

I updated the avt tests to reflect the new min value in the default story. This prop isn't covered by any unit tests right now, I've opened #16207 to track addressing that so we can merge this.