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

Improve number display #3

Closed
halivert opened this issue Oct 25, 2022 · 11 comments · Fixed by #5
Closed

Improve number display #3

halivert opened this issue Oct 25, 2022 · 11 comments · Fixed by #5
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest Issue valid for hacktoberfest

Comments

@halivert
Copy link
Owner

halivert commented Oct 25, 2022

Description

We need to improve the display of numbers so user can easily view the difference between the tens, hundreds, thousands and so on

Acceptance criteria

The display must have a comma when a group of three numbers (before decimal point) is displayed

Examples

Screen Shot 2022-10-25 at 9 25 03

Screen Shot 2022-10-25 at 9 25 42

@halivert halivert added enhancement New feature or request good first issue Good for newcomers hacktoberfest Issue valid for hacktoberfest labels Oct 25, 2022
@YashJain2409
Copy link
Contributor

Can i work on this?

@halivert
Copy link
Owner Author

Sure @YashJain2409 👍🏽

@YashJain2409
Copy link
Contributor

Hey @halivert , i have created pr ,you can review it.

@dpuentel dpuentel mentioned this issue Oct 25, 2022
@dpuentel
Copy link
Contributor

This doesn't work as expected.

  • Input "9999" the app show "9999" when it should be "9.999".
  • Input "99999" the app show "99.999" when ir should be "99,999".

The dot, only there be to decimals otherwise the result will be unexpected

@halivert
Copy link
Owner Author

Hey @dpuentel, maybe the locale of your browser is different because this is implemented with toLocaleString so the PR solution is better than the described one, thank you anyway

dpuentel added a commit to dpuentel/calculator-app that referenced this issue Oct 25, 2022
@dpuentel
Copy link
Contributor

Hey @dpuentel, maybe the locale of your browser is different because this is implemented with toLocaleString so the PR solution is better than the described one, thank you anyway

My browser locale is 'es-ES'.

However, the locale should not influence the result of the operations, and with locale 'es-ES' it is not working correctly.

The locale 'es-ES' uses '.' to separate thousands. This causes an error interpreting it as decimals.

Check 2ad3058, which doesn't affect the values for the calculation, only the display, and also forces the locale to 'en'

@halivert
Copy link
Owner Author

Yes @dpuentel you're right, what do you think of change the decimal point in the keyboard instead?

@halivert halivert reopened this Oct 26, 2022
@YashJain2409
Copy link
Contributor

@halivert Can you explain issue you are getting ?

@halivert
Copy link
Owner Author

Yes @YashJain2409, when you try to use the locale string with locales that use decimal coma, instead decimal point, some issues arise because the point in the calculator keyboard doesn't change.

I hope @dpuentel can upload a gif or screenshot showing us the actual issue.

@dpuentel
Copy link
Contributor

Yes @YashJain2409, when you try to use the locale string with locales that use decimal coma, instead decimal point, some issues arise because the point in the calculator keyboard doesn't change.

I hope @dpuentel can upload a gif or screenshot showing us the actual issue.

Of course. Here are some examples:
low-chrome-capture-2022-9-26 (1)
low-chrome-capture-2022-9-26 (2)
low-chrome-capture-2022-9-26

@YashJain2409
Copy link
Contributor

ok got it thanks for explaining

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest Issue valid for hacktoberfest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants