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

🐞 - Add configurable parameter minusSign to Number #969

Closed
5 of 9 tasks
framasev opened this issue Jan 30, 2024 · 9 comments
Closed
5 of 9 tasks

🐞 - Add configurable parameter minusSign to Number #969

framasev opened this issue Jan 30, 2024 · 9 comments
Assignees
Labels
bug Something isn't working community contribution This issue was closed by a PR from the community P2 This issue has medium priority scope: kit Related to @maskito/kit

Comments

@framasev
Copy link

Which package(s) are the source of the bug?

@maskito/kit

Playground Link

https://stackblitz.com/edit/angular-8ndsd9?file=src%2Fmain.ts

Description

When using the maskitoNumberOptionsGenerator mask, hyphen minus (U+002d) is transformed to minus sign (U+2212) causing coercion errors:

// masked: 8722
console.log('masked: ', maskitoTransform('-1', maskitoNumberOptionsGenerator()).charCodeAt(0)); 
// unmasked: 45
console.log('unmasked: ', '-1'.charCodeAt(0));

// masked to number: NaN
console.log('masked to number: ', Number(maskitoTransform('-1', maskitoNumberOptionsGenerator())));
// unmsaked to number: -1
console.log('unmsaked to number: ', Number('-1'));

For example, these custom validators causing errors because of NaN (and in a server side - illegal character):

export class NumberValidators {
  static number({ value }: AbstractControl) {
    return NumberValidators.isNumber(value)
      ? null
      : { notNumber: `${value} is not a number` };
  }

  static integer({ value }: AbstractControl) {
    return NumberValidators.isInteger(value)
      ? null
      : {
          notInteger: `${value} is not an integer`,
        };
  }

  static natural({ value }: AbstractControl) {
    return NumberValidators.isNumber(value) &&
      NumberValidators.isInteger(value) &&
      value > 0
      ? null
      : { notNaturalNumber: `${value} is not a natural number` };
  }

  static whole({ value }: AbstractControl) {
    return NumberValidators.isNumber(value) &&
      NumberValidators.isInteger(value) &&
      value >= 0
      ? null
      : { notWholeNumber: `${value} is not an whole number` };
  }

  static positive({ value }: AbstractControl) {
    return NumberValidators.isNumber(value) && value > 0
      ? null
      : { notPositiveNumber: `${value} is not a positive number` };
  }

  private static isNumber(num: number | string): boolean {
    return Number.isNaN(Number(num)) === false;
  }

  private static isInteger(num: number | string): boolean {
    return Number.isInteger(Number(num));
  }
}

See tc39/proposals#37.

Maskito version

2.0.0

Which browsers have you used?

  • Chrome
  • Firefox
  • Safari
  • Edge

Which operating systems have you used?

  • macOS
  • Windows
  • Linux
  • iOS
  • Android
@framasev framasev added the bug Something isn't working label Jan 30, 2024
@github-project-automation github-project-automation bot moved this to 💡 Backlog in Taiga-family Jan 30, 2024
@nsbarsukov
Copy link
Member

Use maskitoParseNumber to parse masked string

Screenshot 2024-01-30 at 15 40 40

@framasev
Copy link
Author

@nsbarsukov I would like to use maskitoNumberOptionsGenerator to create and apply mask to input without manual parsing of input value. The input value should be a valid number by default.

@nsbarsukov
Copy link
Member

@nsbarsukov I would like to use maskitoNumberOptionsGenerator to create and apply mask to input without manual parsing of input value. The input value should be a valid number by default.

Mask is used to format a number. Formatted numbers display 'minus' sign, rather than 'hyphen' sign which you would usually expect when typing on a keyboard. This is better for accessibility with screen reader, as well as typographically correct. However it might cause issues when working with the value. Use maskitoParseNumber to turn the input string value into properly parsed number if you need to validate or manipulate it.

@framasev
Copy link
Author

@nsbarsukov I believe that accessibility and typographic correctness should align with the interpreter's behavior and not impose unnecessary challenges on developers. Such behavior should be optional, left to the developer's discretion rather than enforced by default.

At the moment, developers are presented with the following challenges:

  • values are not recognized as valid numbers by the interpreter;
  • numeric validators do not function as expected;
  • manual conversion of values is required.

Why suggest a default behavior that might be more accessible and typographically correct when it essentially serves as a "one-size-fits-all" approach? It seems more prudent to allow developers the flexibility to opt into such behavior based on their specific use cases. This ensures a balance between accessibility, typographic correctness, and developer empowerment.

@nsbarsukov
Copy link
Member

@framasev okey, we will add configurable parameter minusSign to maskitoNumberOptionsGenerator 👌

import {maskitoNumberOptionsGenerator} from '@maskito/kit';

const options = maskitoNumberOptionsGenerator({
    minusSign: '-',
});

It will be remain CHAR_MINUS as default value.
But developer will be able replace it with any value.

@nsbarsukov nsbarsukov changed the title 🐞 - Hyphen minus transforms to minus sign causing coercion errors 🐞 - Add configurable parameter minusSign to Number Jan 30, 2024
@nsbarsukov nsbarsukov added P2 This issue has medium priority scope: kit Related to @maskito/kit labels Jan 30, 2024
@waterplea
Copy link
Contributor

waterplea commented Jan 30, 2024

Your numeric validators wouldn't work as expected anyway, though, because they will be unable to parse thousand separators: Number('10 728.90') -> NaN.

And manual conversion is required anyway if you need a number and not a string.

@framasev
Copy link
Author

framasev commented Jan 30, 2024

@waterplea You are right. In any case, disabling the formatting of thousandSeparator is an option, unlike the formatting of the minus sign.

Regarding the masking directive, does it support different values for the model and the displayed value in the input, relying, for example, on flags like emitModelToViewChange and emitViewToModelChange in the implementation? This way, we could maintain accessibility and typographic correctness while preserving the validity of the underlying value.

@waterplea
Copy link
Contributor

Generally speaking it's impossible to maintain a masked and unmasked couple, and current directive is the general one. A dedicated number directive can easily be implemented extending default accessor:
https://stackblitz.com/edit/maskito-number-input

@nsbarsukov
Copy link
Member

Closed by #1118

@github-project-automation github-project-automation bot moved this from 💡 Backlog to ✅ Done in Taiga-family Mar 7, 2024
@waterplea waterplea added the community contribution This issue was closed by a PR from the community label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working community contribution This issue was closed by a PR from the community P2 This issue has medium priority scope: kit Related to @maskito/kit
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants