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

ONL-4443: fix: Fixed the way how decimal separator and group separator are detected #156

Merged
merged 4 commits into from
May 5, 2020

Conversation

mcibique
Copy link
Contributor

@mcibique mcibique commented May 4, 2020

Amount input uses Intl API for detecting decimal and grouping separator:

  function getSeparator(locale, type) {
    return new Intl.NumberFormat(locale)
      .formatToParts(1111.1)
      .find(part => part.type === type)
      .value;
  }

getSeparator('en', 'decimal') // '.'
getSeparator('en', 'group') // ','

But the problem is that formatToParts doesn't exist on IE11. There is a polyfill but not for NumberFormat. So I wrote our own detection and validated it against all locales in the world that are shipped with intl package.

I found strange behaviour of my functions for Swedish locale, where getGroupingSeparator returns ',' instead of empty space . I run the same test with formatToParts function and I got the same bug. This is happening only on Chromium browsers, while Firefox and IE11! got it right. It seems like there is a bug in Chromium not having some locales installed and it falls back to en-US settings. Solution for this would be to start using Intl polyfill on every single browser and ignore built-in Intl:

<script src="https://cdn.polyfill.io/v2/polyfill.min.js?features=Intl.~locale.en"></script>

^^ that's 70KB polyfill, 15.5KB gzipped.

@mcibique mcibique requested a review from a team as a code owner May 4, 2020 14:52
@vercel
Copy link

vercel bot commented May 4, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ebury/chameleon/asnpy8sfw
✅ Preview: https://chameleon-git-onl-4443-fix-currency-input-not-working-in-ie11.ebury.now.sh

@mcibique mcibique self-assigned this May 4, 2020
@matallo
Copy link
Contributor

matallo commented May 4, 2020

Splendid work 👏

I know we might be penalizing 🇸🇪 clients, but would it be possible to just add the polyfill for those?

@mcibique
Copy link
Contributor Author

mcibique commented May 4, 2020

I'm not sure it will that easy. I checked how many locales are affected by this bug: 37!

Locales which should be using empty space:

af, be, br, eo, ff, ka, kk, ky, nn, os, se, sq, tk, uz

Locales which should be using dot:

az, bs, eu, fo, fy, gl, hy, is, kl, km, lb, ln, lo, lu, mk, rn, rw, sg

Locales which should be using apostrophe:

ar, ks, ps

Locales which should be using different kind of apostrophe:

rm

and then there is fr which separator Chromium identifies correctly as a space but it's a wrong space.

I wonder if this bug happens to somebody who installed Chrome/Chromium in Sweden ...

@mcibique
Copy link
Contributor Author

mcibique commented May 4, 2020

I also discovered that "se" locale is not the entire Sweden (that's "sv"), but it's a nation/community/people called Sami which live in northern part of Sweden. SV locale works well.

@matallo
Copy link
Contributor

matallo commented May 5, 2020

ugh, way too many edge cases, shall we add the polyfill always then? thanks for the investigative work 🕵️

@mcibique
Copy link
Contributor Author

mcibique commented May 5, 2020

I wouldn't TBH, none of these locales are supported by EBO. It might be a problem for somebody who uses this library in their application and needs proper support for all of them. In that case I'd recommend them to polyfill it properly in their app and don't ship any polyfill with chameleon-components. Maybe we should write a note in README about this issue.

@mcibique mcibique force-pushed the ONL-4443-fix-currency-input-not-working-in-ie11 branch from 584a0ed to 2c63329 Compare May 5, 2020 13:04
@matallo
Copy link
Contributor

matallo commented May 5, 2020

good call, let's go with the note in the README 👍

@mcibique mcibique force-pushed the ONL-4443-fix-currency-input-not-working-in-ie11 branch from 9145d1f to a1936b1 Compare May 5, 2020 14:49
@mcibique mcibique merged commit ea3cc65 into master May 5, 2020
@mcibique mcibique deleted the ONL-4443-fix-currency-input-not-working-in-ie11 branch May 5, 2020 14:54
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.

2 participants