modus-web-components icon indicating copy to clipboard operation
modus-web-components copied to clipboard

NumberInput: Add decimal type and currency

Open ElishaSamPeterPrabhu opened this issue 1 year ago • 10 comments

Description

  • With cleave-zen added decimal to help with localisation of numbers

References Fixes #2440

Type of change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [x] Documentation update

How Has This Been Tested?

https://deploy-preview-2634--moduswebcomponents.netlify.app/?path=/story/user-inputs-number-input--default

Checklist

  • [x] My code follows the style guidelines of this project
  • [ ] I have performed a self-review of my own code
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes
  • [ ] Any dependent changes have been merged and published in downstream modules
  • [x] I have checked my code and corrected any misspellings

ElishaSamPeterPrabhu avatar Jun 24 '24 04:06 ElishaSamPeterPrabhu

Deploy Preview for moduswebcomponents failed. Why did it fail? →

Name Link
Latest commit 870c77102de79f6683581911c86cb2d49344b4d6
Latest deploy log https://app.netlify.com/sites/moduswebcomponents/deploys/66d07a6be9a92a000865270a

netlify[bot] avatar Jun 24 '24 04:06 netlify[bot]

  1. Please change inputmode to decimal (not numeric).
  2. Please fix this accessibility error. image

coliff avatar Jun 26 '24 06:06 coliff

  1. decimal

Including the cleave-zen library the input is required to have type text , hence the aria attributes like

 aria-valuemax={this.maxValue}
 aria-valuemin={this.minValue}
 aria-valuenow={this.value} 

are not valid , so can we remove them or is their any alternative? @coliff

ElishaSamPeterPrabhu avatar Jun 26 '24 10:06 ElishaSamPeterPrabhu

Including the cleave-zen library the input is required to have type text , hence the aria attributes like

 aria-valuemax={this.maxValue}
 aria-valuemin={this.minValue}
 aria-valuenow={this.value} 

are not valid , so can we remove them or is their any alternative? @coliff

Yep, please remove them. They are not valid. Thanks!

coliff avatar Jun 26 '24 10:06 coliff

Including the cleave-zen library the input is required to have type text , hence the aria attributes like

 aria-valuemax={this.maxValue}
 aria-valuemin={this.minValue}
 aria-valuenow={this.value} 

are not valid , so can we remove them or is their any alternative? @coliff

Yep, please remove them. They are not valid. Thanks!

Removed them.

ElishaSamPeterPrabhu avatar Jun 26 '24 10:06 ElishaSamPeterPrabhu

The number input PR is good in many ways, but I have some concerns... Demo: https://deploy-preview-2634--moduswebcomponents.netlify.app/?path=/docs/user-inputs-number-input--default

  • It uses the input type="text" now instead of type="number", so user can't use mouse scroll wheel to increase/decrease and the arrow buttons to adjust doesn't display. Maybe a minor thing?
  • If you see Number Input Demo 1 - if this was for a credit card you wouldn't want commas or dots in the input. I think separators should be opt-in rather than on by default.
  • DecimalPlaces is an optional attribute so I suggest it is off by default can be.
  • Unable to enter a numeric input such as 0123456789 - this could be a valid use for a part number or something.

I think to summarize, I think the defaults should be like what they were before and new functionality should be opt-in.... otherwise it could cause breaking changes / unintended side effects for users web apps.

coliff avatar Jun 26 '24 14:06 coliff

The number input PR is good in many ways, but I have some concerns... Demo: https://deploy-preview-2634--moduswebcomponents.netlify.app/?path=/docs/user-inputs-number-input--default

  • It uses the input type="text" now instead of type="number", so user can't use mouse scroll wheel to increase/decrease and the arrow buttons to adjust doesn't display. Maybe a minor thing?
  • If you see Number Input Demo 1 - if this was for a credit card you wouldn't want commas or dots in the input. I think separators should be opt-in rather than on by default.
  • DecimalPlaces is an optional attribute so I suggest it is off by default can be.
  • Unable to enter a numeric input such as 0123456789 - this could be a valid use for a part number or something.

I think to summarize, I think the defaults should be like what they were before and new functionality should be opt-in.... otherwise it could cause breaking changes / unintended side effects for users web apps.

Addressed all this comments

  • Added manual scrolling ability along with plus or minus buttons to change number values.
  • Removed commas or dots for the default version
  • Set decimal places as optional attribute by removing default value as 2
  • Enabled the ability to enter 0123456789, by adding property is-credit-card

ElishaSamPeterPrabhu avatar Jul 01 '24 09:07 ElishaSamPeterPrabhu

is-credit-card isn't the right name for that option. Credit cards can't begin with a zero anyway. I'm not sure what the best name would be though ... allow-leading-zero ?

coliff avatar Jul 18 '24 13:07 coliff

is-credit-card isn't the right name for that option. Credit cards can't begin with a zero anyway. I'm not sure what the best name would be though ... allow-leading-zero ?

That is a better name , can we go with allow-leading-zero ? @cjwinsor

ElishaSamPeterPrabhu avatar Jul 19 '24 05:07 ElishaSamPeterPrabhu

I think we should be using the Intl.NumberFormat standard js object to handle formatting. The end user would provide a locale and the number format options.

I was looking at this library https://dm4t2.github.io/intl-number-input/guide/#installation but i wasn't keen on the format as you type aspect of it... but thats just my opinion. If there was a way to use this library without the live formatting it would be great... otherwise it might be easier to just handle it ourselves. I'm open to using another library if one is found as well that wraps the functionality in a useful way. I just felt the picked library complicated the end user experience too much.

I'd like to reduce the complexity for the end user as we should expect that they will have or otherwise obtain the users locale.

For the component, I envision something like

<modus-number-input type="currency" locale="en-US" currency="USD"></modus-number-input>

behind the scenes we are creating an input with type="text" (keep it as number otherwise), and we are doing something to the affect of

Intl.NumberFormat(this.locale, { style: 'currency', currency: this.currency }).format(this.value);

We should probably also map any existing properties to the formatting that control the number of decimals. Additional enhancements could be to restrict them from typing more decimals than they have the precision set to... I've seen libraries take a number with decimal places like 1234.5 and do something like

const formatter = new Intl.NumberFormat(this.locale);
const parts = formatter.formatToParts(1234.5);

let decimalPoint = '.';
let groupSeparator = ',';

for (const part of parts) {
  if (part.type === 'decimal') {
    decimalPoint = part.value;
  }
  if (part.type === 'group') {
    groupSeparator = part.value;
  }
}

Once you know those, you can do cool things like prevent them from typing more than max decimal digits after the decimalPoint.

Additional thoughts are that we can leverage this for a type=decimal. Additional props can be added as needed later on, but I think just locale and currency are enough to satisfy having a currency input initially. Also helps we can use navigator.language to get the locale from the browser... not sure if modus consumers would want... but might be a good default if they don't pass in a locale. @prashanth-offcl

cjwinsor avatar Aug 06 '24 21:08 cjwinsor

Merged in #2864

prashanth-offcl avatar Sep 16 '24 10:09 prashanth-offcl