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

[localization] Add currency, separators, isMetric and update region #11663

Merged
merged 24 commits into from
Feb 23, 2021

Conversation

IjzerenHein
Copy link
Contributor

@IjzerenHein IjzerenHein commented Jan 18, 2021

Why

Adds the region property for Android and 4 new properties: currency, decimalSeparator, groupingSeparator and isMetric.

Replaces and closes #10354

How

  • Add region property for Android
  • Add currency property for iOS and Android
  • Add decimalSeparator property for iOS, Android and web
  • Add groupingSeparator property for iOS, Android and web
  • Add isMetric property for iOS, Android and web
  • Fix region property on web when locale contained variants or both region and script
  • Update changelog

Test Plan

  • Added tests for all new properties
  • All tests succeed
  • Debugged iOS and Android to verify results

@github-actions
Copy link
Contributor

Native Component List for this branch is ready

@IjzerenHein IjzerenHein changed the title [localization] Add region for Android and 4 new fields [localization] Add currency, separators, isMetric and update region Jan 19, 2021
@IjzerenHein
Copy link
Contributor Author

friendly ping to @bbarthec :)

docs/pages/versions/unversioned/sdk/localization.md Outdated Show resolved Hide resolved
docs/pages/versions/unversioned/sdk/localization.md Outdated Show resolved Hide resolved
docs/pages/versions/unversioned/sdk/localization.md Outdated Show resolved Hide resolved
docs/pages/versions/unversioned/sdk/localization.md Outdated Show resolved Hide resolved
docs/pages/versions/unversioned/sdk/localization.md Outdated Show resolved Hide resolved
packages/expo-localization/src/Localization.ts Outdated Show resolved Hide resolved
@IjzerenHein
Copy link
Contributor Author

I think we should go with Option B (the code in this PR) because it lines up with how most of the world works, aside from iOS, and our intent is to create a universal API instead of exposing native APIs. If we want to expose the iOS-specific locale ID, that should go under a separate property named platformLocale that is documented to return a platform-specific string with no guarantee it with be consistent across platforms.

Sounds good. I've made the suggested changes, thank you for reviewing.
I'm in favour of not adding platformLocale unless there is a good reason why people should need this information. So for now, I'd leave it out.

@IjzerenHein IjzerenHein requested a review from ide February 19, 2021 14:16
IjzerenHein and others added 16 commits February 22, 2021 16:55
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
…ive.ts

Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
Co-authored-by: James Ide <ide@users.noreply.github.com>
…ive.ts

Co-authored-by: James Ide <ide@users.noreply.github.com>
IjzerenHein and others added 2 commits February 23, 2021 09:40
Co-authored-by: James Ide <ide@users.noreply.github.com>
@IjzerenHein IjzerenHein merged commit d1736fc into master Feb 23, 2021
@IjzerenHein IjzerenHein deleted the @hein/localization branch February 23, 2021 10:34
@chybisov
Copy link

chybisov commented Apr 15, 2021

@IjzerenHein hi, this probably not the right place to ask, but could you please add is24Hour format property? Thank you!

@IjzerenHein
Copy link
Contributor Author

@chybisov This PR already been merged. If you want that feature added, please create a PR or request the feature on https://expo.canny.io/

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

Successfully merging this pull request may close these issues.

3 participants