Skip to content
This repository has been archived by the owner on Jun 16, 2022. It is now read-only.

LL-6732 regions, dates, time and currencies #1941

Merged
merged 27 commits into from
Feb 1, 2022
Merged

Conversation

ofreyssinet-ledger
Copy link
Contributor

@ofreyssinet-ledger ofreyssinet-ledger commented Oct 13, 2021

Edit (31/01):

Added a "searchable" option to the selection screen for a much better UX:

Simulator.Screen.Recording.-.iPhone.12.-.2022-01-31.at.11.09.20.mp4

Edit (19/01):

Now ready for dev review 👍


Original PR comment:

  • There is now a "region" settings in the general settings section, it shows the list of all regions
  • That setting sets a locale in settings.locale, separate from the settings.language locale. If it's not defined, the localeSelector falls back to the language locale
  • All numbers and dates are now formatted using that locale.
  • Dates are now all formatted in a numeric way to avoid weird mixes of languages (text in one language and dates in another language).

Screenshot 2021-10-13 at 15 55 11

Screenshot 2021-10-13 at 15 55 23

A few examples on the portfolio screen with fr-FR locale:
Screenshot 2021-10-13 at 16 18 58

Questions remaining:

  • Too expensive -> needs to change very commonly used types in ledgerjs What about the position of the currency unit ? Should there be another PR on live-common to put the currency unit on the left or on the right depending on the locale ? Or is it somehow already implemented but I just didn't find where/how ?
  • DONE The region selection screen is basically a huge flatlist with a ton of entries so it's not such a good experience to scroll to the desired setting. Should I implement the searchable prop marked as a TODO in makeGenericSelectScreen ?
  • There are now 2 distinct locales and 2 way to get them with a hook, I'm worried that the similar naming will bring confusion so feel free to suggest a renaming
    • the i18next locale, for translations, accessible with useLocale() -> I would rename that useTranslationLocale
    • the settings.locale locale, for dates & numbers, accessible with useSelector(localeSelector)

Type

Feature

Context

LL-6732

Parts of the app affected / Test plan

I believe this can only be tested manually and the best scenario would be if we have testers of different nationality testing for their region ...

  • All numbers and dates displayed in the app are affected by the region selected so make sure they are formatted properly and none are forgotten (I tried to cover the entire app by searching in the code for formatting of numbers & dates but I might have missed some spots)
  • Verify that no date is left with "translatable" text inside (in particular months)
  • Inputs for currencies are affected and should behave normally for instance if a user uses "France" as a region and uses a comma as a digit separator.

@ofreyssinet-ledger ofreyssinet-ledger requested review from a team October 13, 2021 14:09
Copy link
Contributor

@LFBarreto LFBarreto left a comment

Choose a reason for hiding this comment

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

Some linting issues to fix
LGTM 👍

@LFBarreto
Copy link
Contributor

@ofreyssinet-ledger still some ci issues on flow sorry

@ofreyssinet-ledger
Copy link
Contributor Author

@LFBarreto am I supposed to just run yarn flow to reproduce this?
I get no errors:

Screenshot 2021-11-19 at 10 58 29

@ofreyssinet-ledger
Copy link
Contributor Author

ofreyssinet-ledger commented Nov 19, 2021

I restarted the CI/test check in case it's flaky
EDIT: same result as in previous test run, idk what to do 🤷‍♂️

@ofreyssinet-ledger
Copy link
Contributor Author

Alleluia, CI/test is passing after a rebase

@ofreyssinet-ledger ofreyssinet-ledger self-assigned this Nov 24, 2021
LFBarreto
LFBarreto previously approved these changes Nov 26, 2021
@@ -0,0 +1,37 @@
/* @flow */
import { connect } from "react-redux";
import _ from "lodash";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import _ from "lodash";
import upperFirst from "lodash/upperFirst";

I don't know if we're doing it somewhere else in the app (which might make that not an issue here) but you could propably just import upperFirst to allow for tree shaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch thanks 👍

Copy link
Contributor Author

@ofreyssinet-ledger ofreyssinet-ledger Jan 19, 2022

Choose a reason for hiding this comment

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

Is there a fundamental difference between doing what you suggested and import { upperFirst } from "lodash" regarding tree shaking ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, unfortunately, you have to get a specific babel plugin I think to get the tree shaking with named exports like that for lodash, unless it's not going to be considered as "dead code" 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok good to know, we'll have to do some cleanup as some point then I think regarding this because in some other in the codebase we use a default import for lodash...
Thanks for the review!

Copy link
Contributor

@lambertkevin lambertkevin left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ofreyssinet-ledger
Copy link
Contributor Author

Rebased and implemented "searchable" option to makeGenericSelectScreen for a much better UX:

Simulator.Screen.Recording.-.iPhone.12.-.2022-01-31.at.11.09.20.mp4

@cgrellard-ledger cgrellard-ledger self-requested a review January 31, 2022 15:28
Copy link
Contributor

@cgrellard-ledger cgrellard-ledger left a comment

Choose a reason for hiding this comment

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

commit f962070

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

Successfully merging this pull request may close these issues.

5 participants