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.

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

NumberInput: Add decimal type and currency #2634

Closed

Conversation

ElishaSamPeterPrabhu
Copy link
Collaborator

@ElishaSamPeterPrabhu ElishaSamPeterPrabhu commented Jun 24, 2024

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)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

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

Checklist

  • 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
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@ElishaSamPeterPrabhu ElishaSamPeterPrabhu linked an issue Jun 24, 2024 that may be closed by this pull request
2 tasks
Copy link

netlify bot commented Jun 24, 2024

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

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

@coliff
Copy link
Member

coliff commented Jun 26, 2024

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

@ElishaSamPeterPrabhu
Copy link
Collaborator Author

  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

@coliff
Copy link
Member

coliff commented Jun 26, 2024

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!

@ElishaSamPeterPrabhu
Copy link
Collaborator Author

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.

@coliff
Copy link
Member

coliff commented Jun 26, 2024

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 coliff requested a review from cjwinsor July 18, 2024 13:49
@coliff
Copy link
Member

coliff commented Jul 18, 2024

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 ?

@ElishaSamPeterPrabhu
Copy link
Collaborator Author

ElishaSamPeterPrabhu commented Jul 19, 2024

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

@cjwinsor
Copy link
Contributor

cjwinsor commented Aug 6, 2024

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

Copy link
Collaborator

@prashanth-offcl prashanth-offcl left a comment

Choose a reason for hiding this comment

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

@ElishaSamPeterPrabhu Have you tried Intl.NumberFormat or toLocalString method of Number? Also,

  • Looks like we are using the text input by default for all cases, better to keep the type as number if currency and locale props are not set
  • Locale & currency symbol - The accepted values need to be documented properly. Make sure the values matches with the options in toLocaleString or create an internal mapping
  • Step up & down behaviors
    • working in reverse when using scroll.
    • Not working with keyboard up and down arrow.
    • Not working without specifying locale.
    • The Prop step is not working.
  • Currency prop is not working without locale prop.
  • Min & Max value prop - Behavior changed, now it accepts any number and automatically sets the respective threshold if out of range
  • Cleanup code - some places in e2e have references to Cleave package which is not used now.

@prashanth-offcl prashanth-offcl marked this pull request as draft August 30, 2024 15:14
@prashanth-offcl
Copy link
Collaborator

Merged in #2864

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input: Add new decimal input type
4 participants