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.

Sign up for GitHub

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 support for paste event given allowedDecimalSeparators #556

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

add support for paste event given allowedDecimalSeparators #556

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5935daf
add support for paste event given allowedDecimalSeparators
Toumash Jun 23, 2021
10b28fa
Update test/library/input_numeric_format.spec.js
Toumash Jun 24, 2021
1375431
fix: pasting with allowedDecimalSeparators #349
Toumash Jun 24, 2021
f98d284
chore(readme): update docs for allowedDecimalSeparators
Toumash Jun 24, 2021
81fce43
change decimalSeparator replace function + add prefix and suffix test…
Toumash Jul 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ In typescript you also have to enable `"esModuleInterop": true` in your tsconfig
| isAllowed | ([values](#values-object)) => true or false | none | A checker function to check if input value is valid or not. If this function returns false, the onChange method will not get triggered |
| renderText | (formattedValue, customProps) => React Element | null | A renderText method useful if you want to render formattedValue in different element other than span. It also returns the custom props that are added to the component which can allow passing down props to the rendered element |
| getInputRef | (elm) => void | null | Method to get reference of input, span (based on displayType prop) or the customInput's reference. See [Getting reference](#getting-reference)
| allowedDecimalSeparators | array of char | none | Characters which when pressed result in a decimal separator. When missing, decimal separator and '.' are used |
| allowedDecimalSeparators | array of char | none | Characters which when pressed or pasted from the clipboard will result in a decimal separator. When missing, decimal separator and '.' are used |

**Other than this it accepts all the props which can be given to a input or span based on displayType you selected.**

Expand Down
10 changes: 10 additions & 0 deletions example/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,16 @@ class App extends React.Component {
/>
</div>

<div className="example">
<h3>Custom allowed decimal separators with decimal precision</h3>
<NumberFormat
value={11.11}
allowedDecimalSeparators={['.', ',']}
decimalSeparator='.'
decimalScale={2}
/>
Toumash marked this conversation as resolved.
Show resolved Hide resolved
</div>

<div className="example">
<h3>Format with pattern : Format credit card in an input</h3>
<NumberFormat format="#### #### #### ####" mask="_" />
Expand Down
9 changes: 9 additions & 0 deletions src/number_format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,15 @@ class NumberFormat extends React.Component {
);
}

/** Check for "paste event" with, replace any allowed decimal separator with desired decimal separator. Issue #349 */
if (!format && decimalScale !== 0 && allowedDecimalSeparators.some(separator => value.indexOf(separator) !== -1)) {
let result = value;
allowedDecimalSeparators.forEach(v => {
result = result.replace(v, decimalSeparator);
})
value = result;
}
Copy link
Owner

@s-yadav s-yadav Jun 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  • 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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@s-yadav s-yadav Jun 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

#324 (comment)

Copy link
Collaborator

@nikhil-varma nikhil-varma Jul 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 🤔

Copy link
Author

@Toumash Toumash Jul 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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=" "
      />

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, gonna provide a new version in the next days


const leftBound = !!format ? 0 : prefix.length;
const rightBound = lastValue.length - (!!format ? 0 : suffix.length);

Expand Down
16 changes: 16 additions & 0 deletions test/library/input_numeric_format.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,22 @@ describe('Test NumberFormat as input with numeric format options', () => {
expect(wrapper.state().value).toEqual('23,4456 Sq. ft');
});


it('should work with the configured decimal separators based on the allowedDecimalSeparators prop. Issue #349', () => {
const wrapper = shallow(
<NumberFormat
displayType="input"
thousandSeparator={' '}
prefix=""
allowedDecimalSeparators={['.', ',']}
decimalSeparator='.'
decimalScale={2}
/>,
);
simulateKeyInput(wrapper.find('input'), '123,45');
expect(wrapper.state().value).toEqual('123.45');
});

it('should not break if suffix/prefix has negation sign. Issue #245', () => {
const wrapper = shallow(<NumberFormat suffix="-" />);

Expand Down