add support for paste event given allowedDecimalSeparators

[Toumash]

Describe the issue/change

Described here #349 by @tenkij

Add CodeSandbox link to illustrate the issue (If applicable)

Codesandbox by @tenkij

Describe specs for failing cases if this is an issue (If applicable)

allowedDecimalSeparators={[",", "."]}

decimalSeparator='.'

1. paste '11.11', result '11.11';
2. paste '11,11', result '1 111'

decimalSeparator=','

1. paste '11.11', result '1 111';
2. paste '11,11', result '11,11'

Describe the changes proposed/implemented in this PR

Now the paste works with both cases

H3ZTlYLnmQ.mp4

Please check which browsers were used for testing

• Chrome
• Chrome (Android)
• Safari (OSX)
• Safari (iOS)
• Firefox
• Firefox (Android)

Quick hack-solution before it gets merged

const DemoField =  useMemo(() => NumberFormatCommaPasteHackTextField(setValue), []);

const NumberFormatCommaPasteHackTextField = (setValue) =>
  (props) => {
    return <TextField
      {...props}
      onPaste={(e) => {
        let pastedText = e.clipboardData.getData('text');
        if (pastedText.indexOf(',') !== -1) {
          e.preventDefault();
          setValue(pastedText.replace(',', '.'));
        }
      }}
    />
  }

return  <NumberFormat
        customInput={DemoField}
        allowedDecimalSeparators={[",", "."]}
        decimalSeparator={'.'}
        thousandSeparator=" "
      />

[nikhil-varma]

Can you also please add a related documentation in README.md file for this prop?

Also, although things look good, I would still prefer another review done by @s-yadav to ensure all perspectives are considered and we don't miss anything. I appreciate your patients on this! Thanks!

[Toumash]

Can you also please add a related documentation in README.md file for this prop?

Also, although things look good, I would still prefer another review done by @s-yadav to ensure all perspectives are considered and we don't miss anything. I appreciate your patients on this on this! Thanks!

Done. Waiting for the second review :)

[s-yadav]

• The logic can be simplifed using regex, instead of array.

const decimalSeparatorRegex = new RegExp(allowedDecimalSeparators.map(escapeRegExp).join('|'), 'g');

• If we add this the previous if condition is no longer needed.
• The replace method should happen only on characters between prefix and suffix. What if prefix or suffix has a decimal separator character. like suffix .sqft. Can we add a spec to verify that? <- Add decimal char on the prefix, as replace replaces first char.

[s-yadav]

Also, while looking into code, I feel this transformation is non needed to be done on correctInputValue. Instead we should do it in formatInput method, after this.
https://github.com/s-yadav/react-number-format/blob/master/src/number_format.js#L621

That way we don't have to worry about prefix and suffix. Also, we can remove transformation for allowedDecimalSeparator in line 663.
https://github.com/s-yadav/react-number-format/blob/master/src/number_format.js#L663

[s-yadav]

correctInputValue, is mostly a supporting function for keyDown event. This was introduced due to android keyboard bug, where it doesn't give the correct character code on the keyDown event. Not sure if it still exist.

[s-yadav]

On further discussion looks like the paste event is tricky, what if the pasted text has a thousand separator on it. For example, 1,111.11 This logic will break.

Also, how do you identify if , in pasted character is supposed to be thousand separator or decimal separator. For example what we should treat , in this. 1,111.?

[Toumash]

On further discussion looks like the paste event is tricky, what if the pasted text has a thousand separator on it. For example, 1,111.11 This logic will break.

Also, how do you identify if , in pasted character is supposed to be thousand separator or decimal separator. For example what we should treat , in this. 1,111.?

As far as i know you cannot have allowedDecimalSeparator same as thousand separator. I believe decimal separator difference is more common than thousand separator differences.
When you look at the current library props it looks like you can enter different decimal place separator, but only one thousand separator - and that should stay that way

[s-yadav]

Validation might not be possible as there is a valid usecase for the conflict.

#324 (comment)

[nikhil-varma]

Oh yeah you are right! But then if we just add it in this particular context, this specific use-case might fail? It'll mostly be a bandage over the issue 🤔

[Toumash]

Okay, maybe we should just add this workaround into the README.md then?

const DemoField =  useMemo(() => NumberFormatCommaPasteHackTextField(setValue), []);

const NumberFormatCommaPasteHackTextField = (setValue) =>
  (props) => {
    return <TextField
      {...props}
      onPaste={(e) => {
        let pastedText = e.clipboardData.getData('text');
        if (pastedText.indexOf(',') !== -1) {
          e.preventDefault();
          setValue(pastedText.replace(',', '.'));
        }
      }}
    />
  }

return  <NumberFormat
        customInput={DemoField}
        allowedDecimalSeparators={[",", "."]}
        decimalSeparator={'.'}
        thousandSeparator=" "
      />

[nikhil-varma]

Adding it to the documentation may be an issue because it would become a suggested approach and the actual issue won't be resolved.
Let's do this

• Change documentation for allowedDecimalSeparators saying that paste may not work as expected if there is a conflict between thousandSeparator and allowedDecimalSeparators. Let's also link this PR there in the documentation to provide the rationale behind it.
• We can add a conflict check in your logic between thousandSeparator and allowedDecimalSeparators as @s-yadav pointed out.

If there is no conflict then the paste will work as expected but if there is it may not as suggested above.

[Toumash]

Okay, gonna provide a new version in the next days

[Misiu]

@s-yadav can we ask for your review on this? This would be a very helpful feature. Thank you

[sowtame]

any updates? )

[rhmnaulia]

any updates?

[Toumash]

Nope, feel free to take it over

[Patriksafar]

The issue still persist in latest version - @s-yadav Can I help you to finish this PR?