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 Issue #875 #974

Merged
merged 3 commits into from Feb 12, 2022
Merged

Fix Issue #875 #974

merged 3 commits into from Feb 12, 2022

Conversation

wd021
Copy link
Contributor

@wd021 wd021 commented Feb 9, 2022

Localization error is causing languages that use , instead of . to break (ie de_DE). Applying "en-US" in toLocaleString() to prevent this error.

@wd021 wd021 requested a review from a team as a code owner February 9, 2022 18:25
@Kupuyc
Copy link
Contributor

Kupuyc commented Feb 9, 2022

Is it good approach - to hardcode locale? 🤔

Probably, it's better to use dotenv and set LC_ALL to "en_US.UTF8", "de_DE.UTF8", etc. Doing so we can test localized messages by equivalence classes.

Edit: fix typo.

@wd021
Copy link
Contributor Author

wd021 commented Feb 9, 2022

Hardcoding because we're using en-US.UTF8 formatting for numerical displays.

Not sure what would be the need for setting de_DE.UTF8 or any other local format, it'd just fail the test. Unless we're localizing the whole app to account for different formats 🤷

@NullSoldier
Copy link
Contributor

NullSoldier commented Feb 10, 2022

I think this is going to make people in non en-us countries very unhappy. What if we just make the test use toLocaleString() as well?

@wd021
Copy link
Contributor Author

wd021 commented Feb 10, 2022

good point!

updated - tests now use toLocaleString().

@NullSoldier
Copy link
Contributor

Looks good, will merge if tests pass.

@wd021
Copy link
Contributor Author

wd021 commented Feb 11, 2022

@NullSoldier updated fix for lint issues.

@NullSoldier
Copy link
Contributor

Can you base this off staging?

- need to use toLocaleString() in currency display tests. otherwise we'll run in to localization errors.
@wd021 wd021 changed the base branch from master to staging February 11, 2022 20:22
@wd021
Copy link
Contributor Author

wd021 commented Feb 11, 2022

yes sir

Copy link
Member

@dguenther dguenther left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@dguenther dguenther merged commit ed882a5 into iron-fish:staging Feb 12, 2022
@leanthebean leanthebean added the testnet-credit Anything related to getting testnet points label Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testnet-credit Anything related to getting testnet points
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants