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

feat(FormFields): use rifm to format CreditCard and Decimal fields #306

Merged
merged 9 commits into from
Aug 2, 2021

Conversation

anabelengp
Copy link
Member

@anabelengp anabelengp commented Jul 28, 2021

related to #191

Applied to CreditCardInput and DecimalInput (used by CvvField). Not applied to CreditCardExpirationField since it provokes more problems than what it solves...
before
creditcardfields-before-rifm

after
creditcardfields-after-rifm

@anabelengp anabelengp requested a review from atabel July 28, 2021 11:58
const format = (s?: string) => {
const sanitizedNumber = String(s ?? '').replace(/[^\d]/g, '');
return sanitizedNumber.match(/.{1,4}/g)?.join(' ') ?? sanitizedNumber;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the code of format function, now it is based on formatIban implementation, to keep them similar: https://github.com/Telefonica/mistica-web/blob/master/src/iban-field.tsx#L76-L79

@github-actions
Copy link

github-actions bot commented Jul 28, 2021

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-web-czm4dq7kr-atabel.vercel.app

Built with commit cdfa0e4.
This pull request is being automatically deployed with vercel-action

defaultValue={defaultValue === undefined ? undefined : format(defaultValue)}
ref={inputRef}
maxLength={getCreditCardNumberLength(rifm.value) + 3} // We have to take in account formatting spaces
onChange={rifm.onChange}
Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the event to listen, as I think onChange event is preferred to onInput in react, and I've seen it used in other similar components

@github-actions
Copy link

github-actions bot commented Jul 28, 2021

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

@anabelengp anabelengp force-pushed the anabelengp/WEB-83-rifm-lib-in-credit-card branch from 749c6ce to 33a958b Compare July 29, 2021 11:04
@@ -48,19 +56,44 @@ export const DecimalInput: React.FC<DecimalInputProps> = ({inputRef, value, defa
return parts.shift() + localDecimalChar + parts.join('');
};

const replace = (value: any) => String(value ?? '').replace(/[.,]/g, localDecimalChar); // use local decimal char
Copy link
Member Author

Choose a reason for hiding this comment

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

replace the localDecimalChar in a replace input for rifm to prevent the caret from being kept in the position before the delimiter: realadvisor/rifm#65 (comment)

@anabelengp anabelengp marked this pull request as ready for review July 29, 2021 11:22
Copy link
Collaborator

@atabel atabel left a comment

Choose a reason for hiding this comment

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

I've seen a weird behavior in DecimalFiled:

  • write 1,23456
  • place the cursor between 2 and 3
  • start pressing the . key
    The cursor moves one position on every keystroke and an onChangeValue receives a value with an additional comma

src/__acceptance_tests__/form-fields-acceptance-test.tsx Outdated Show resolved Hide resolved
const format = (value: any) => {
const sanitized = String(value ?? '').replace(/[^.,\d]/g, ''); // remove non digits or decimal separator chars;
const firstSeparator = /[.,]/.exec(sanitized);
const parts = sanitized.split(/[.,]/);
Copy link
Member Author

Choose a reason for hiding this comment

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

do not make the localDecimalChar replacement here, instead, keep the one the user typed. Make that replacement in replace prost-processor

@anabelengp anabelengp requested a review from atabel July 30, 2021 11:18
@atabel
Copy link
Collaborator

atabel commented Jul 30, 2021

image
I think we shouldn't allow a comma as first char

@atabel
Copy link
Collaborator

atabel commented Jul 30, 2021

why don't we use IntegerField instead of DecimalField in CvvField? :/

Copy link
Collaborator

@atabel atabel left a comment

Choose a reason for hiding this comment

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

@anabelengp
Copy link
Member Author

mm I thought accepting a comma as first char was an agreed behaviour. I saw this other implmementation which I think is more convenient: https://realadvisor.github.io/rifm/. It is more restrictive.

// allow only one floating point
format={v => (v.match(/\d+[.,]?\d*/) || []).join('')}

Shall I change it??

@atabel
Copy link
Collaborator

atabel commented Jul 30, 2021

mm I thought accepting a comma as first char was an agreed behaviour. I saw this other implmementation which I think is more convenient: realadvisor.github.io/rifm. It is more restrictive.

// allow only one floating point
format={v => (v.match(/\d+[.,]?\d*/) || []).join('')}

Shall I change it??

hmmm... well I'm not sure to be honest. Maybe we could accept '.123' as a valid version of '0.123'. Calculator use to accept that, so I think it could be fine.

@atabel atabel requested review from pladaria and atabel July 30, 2021 15:26
@pladaria pladaria changed the title WEB-83 use rifm lib in CreditCard fields feat(FormFields): use rifm to format CreditCard and Decimal fields Aug 2, 2021
@pladaria pladaria merged commit dd2f8ff into master Aug 2, 2021
@pladaria pladaria deleted the anabelengp/WEB-83-rifm-lib-in-credit-card branch August 2, 2021 13:04
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 9.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants