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

No-state-locale-separators #164

Merged
merged 4 commits into from
Oct 23, 2023
Merged

No-state-locale-separators #164

merged 4 commits into from
Oct 23, 2023

Conversation

EvgeniiaVak
Copy link
Member

@EvgeniiaVak EvgeniiaVak commented Oct 22, 2023

fixes #76
fixes #158

fliebenberg and others added 2 commits October 21, 2023 14:45
fixes tests and displayAmount usage
one test is still failing
because displayAmount logic seems to be incorrect
@netlify
Copy link

netlify bot commented Oct 22, 2023

Deploy Preview for chipper-sunburst-578cfe ready!

Name Link
🔨 Latest commit 9bbbdb1
🔍 Latest deploy log https://app.netlify.com/sites/chipper-sunburst-578cfe/deploys/65362f4031718b000891c898
😎 Deploy Preview https://deploy-preview-164--chipper-sunburst-578cfe.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@EvgeniiaVak
Copy link
Member Author

This is an alternative approach to #159 - I just copied your commit, but removed redux to avoid testing import errors

One test still fails, but this time correctly - due to logic error in the displayAmount

    Expected: "1234"
    Received: "1.2K"

@fliebenberg I believe it would be easier for you to fix this error, you can push directly to this branch if you are ok with this approach.

@fliebenberg
Copy link
Contributor

I have added decimalSeparator and thousandsSeparator as 2 optional parameters to the displayAmount function. This adds nice functionality in case someone wants to be explicit about the separators, but more importantly it helps to make sure the tests can work. The problem in the test case you flagged was that although the test was written for cases where there is no thousandsSeparator, there was no way to explicitly force no thousandsSeparator. This is where the optional parameters in the function helps. All the tests work now again as expected.
I also just removed the uiSlice import from the store.js file.

Copy link
Contributor

@fliebenberg fliebenberg left a comment

Choose a reason for hiding this comment

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

Everything seems to be working as expected now. displayAmount automatically uses locale specific separators unless the caller specifically provides separators.
All tests pass and app compiles without problem.

@EvgeniiaVak EvgeniiaVak marked this pull request as ready for review October 23, 2023 09:08
@EvgeniiaVak EvgeniiaVak merged commit 35b4805 into main Oct 23, 2023
4 checks passed
@EvgeniiaVak EvgeniiaVak deleted the no-state-locale-separators branch October 23, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Read locale separators for displayAmount Decide what to use: . or , in numbers and test input fields
2 participants