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

Fix: disable currency abbreviations in custom amounts #7625

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

JoshuaHungDinh
Copy link
Contributor

@JoshuaHungDinh JoshuaHungDinh commented Nov 15, 2024

Resolves #7537
Resolves GIVE-1985

Description

References:
cchanxzy/react-currency-input-field#255
https://github.com/cchanxzy/react-currency-input-field#abbreviations

Some languages in certain browsers update the CustomAmount on forms with abbreviated letters for the value. This results in an issue where the input can render NaN & adds "000" for every additional 0 included.

This PR addresses the issue by disabling the abbreviations.

Affects

Donation Forms

Visuals

Disable Abbreviations

Screen.Recording.2024-11-15.at.6.58.00.AM.mov

With calculated separators

Screen.Recording.2024-11-21.at.10.51.33.AM.mov

Testing Instructions

  • Create a form.
  • Using Chrome browser change the preferred language to Swedish.
  • Update currency from settings to Swedish krona/SEK.
  • View Form and enter amounts in the custom field, were testing that the correct number of "0"s are added each time and that NaN errors do not occur.

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Self Review of code and UX completed

@JoshuaHungDinh JoshuaHungDinh changed the title Fix: support currency abbreviations in custom amounts Fix: disable currency abbreviations in custom amounts Nov 15, 2024
@JoshuaHungDinh JoshuaHungDinh marked this pull request as ready for review November 15, 2024 15:33
@kjohnson
Copy link
Member

@JoshuaHungDinh the screen recording in the PR description suggests that the abbreviation is still used. Is that correct?

@JoshuaHungDinh
Copy link
Contributor Author

@kjohnson I logged a couple of items coming from the input field. We are only using the value. It looks like it might replace the letter with a space in the display.

Screenshot 2024-11-15 at 8 42 55 AM

@kjohnson
Copy link
Member

Oh, ok, so the underlying issue was that it was interpreting the k as an abbreviations for thousand, which was being converted from 1k to 1,000. Interesting. That would explain why it was intermittent based on the currency.

Abbreviations

It can parse values with abbreviations k, m and b

Examples:

1k = 1,000
2.5m = 2,500,000
3.456B = 3,456,000,000

This can be turned off by passing in disableAbbreviations.

@JoshuaHungDinh
Copy link
Contributor Author

JoshuaHungDinh commented Nov 19, 2024

@kjohnson Yea, Im not the biggest fan of the formatted display not reintroducing the proper group separator. There was another suggestion to calculate the group and decimal separators outside of react-currency-input & then provide them to the component as props. I've updated this PR to include both suggestions.

cchanxzy/react-currency-input-field#266

@kjohnson kjohnson requested review from kjohnson and removed request for pauloiankoski December 2, 2024 16:46
@kjohnson
Copy link
Member

kjohnson commented Dec 4, 2024

There was another suggestion to calculate the group and decimal separators outside of react-currency-input & then provide them to the component as props.

@JoshuaHungDinh is this a second, related issue being addressed?

const [decimalSeparator, setDecimalSeparator] = useState('.');

useEffect(() => {
const formatter = new Intl.NumberFormat();
Copy link
Member

Choose a reason for hiding this comment

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

This will override the browser's locale and assume a US locale for separators.

While is does prevent the NaN issue, we shouldn't do so by locking the local config.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I don't think we need a hook for this, since the value shouldn't change after page load. Instead, we could initialize these variables at the top of the component file,

import ...

const formatter = new Intl.NumberFormat();
// ...

const Component = () => {
    // ...
}

export default Component;

Copy link
Member

Choose a reason for hiding this comment

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

@JoshuaHungDinh I updated this to use the browser locale configuration and replaced the non-breaking space on the group separator to avoid any conflicts with the suffix separator used by the <CurrencyInput /> component.

const formatter = new Intl.NumberFormat(navigator.language);
const groupSeparator = formatter.format(1000).replace(/[0-9]/g, '');
const decimalSeparator = formatter.format(1.1).replace(/[0-9]/g, '');

  //
  groupSeparator={
    /**
     * Replace non-breaking space to avoid conflict with the suffix separator.
     * @link https://github.com/cchanxzy/react-currency-input-field/issues/266
     */
    groupSeparator.replace(/\u00A0/g, ' ')
  }
  //

Copy link
Contributor Author

@JoshuaHungDinh JoshuaHungDinh Dec 9, 2024

Choose a reason for hiding this comment

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

Awesome, these changes make sense.

Thanks for helping me move this forward. I'll send it to QA.

Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

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

@JoshuaHungDinh good job researching the underlying conflict!

@kjohnson
Copy link
Member

kjohnson commented Dec 6, 2024

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.

[Broken functionality] Entering currency amount in cs_CZ locale does not behave correctly
2 participants