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

Update countUp.ts - support large integer numbers #289

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shani-360
Copy link

I'm submitting a...

[v ] Bug Fix

Description

Large number keep original number without changing the value

Does this PR introduce a breaking change?

[ ] Yes
[v] No

@inorganik
Copy link
Owner

Can you please show me how to reproduce this issue? I'm not clear what you are fixing.

@shani-360
Copy link
Author

shani-360 commented Oct 11, 2022

Hi, for example:

[countUp] : 8889815748235620000

Will display the value as 8,889,815,748,235,620,352

(Will add extra 352 to the value)

@inorganik
Copy link
Owner

You found a legit bug, but I hesitate to even create an issue because I don't know a use case for it. The issue comes down to the fact that extremely large numbers in programming need special handling. Javascript has some ways to deal with this - (see Number.MAX_SAFE_INTEGER) but CountUp doesn't handle it.

I appreciate your contribution, but your fix has some issues. There's a syntax error (backwards paren) on line 229. If you followed contribution guidelines, the tests would've caught this 🙂 . But even despite that the fix introduces errors for normal use cases like counting to 3000: because of the change in the count() method, the number cannot get rounded to the nearest integer if the options have decimalPlaces set to 0, and they get printed as floats during the animation. Here's a stackblitz where I was testing your fix: https://stackblitz.com/edit/countup-typescript-5zvtbd?file=index.html

If you have a real use case then please do create an issue, and steps to reproduce. Then if you want maybe take a more careful look at fixing it in this PR, accounting for the contributing guidelines and various use-cases.

@inorganik inorganik self-requested a review October 12, 2022 22:41
@shani-360 shani-360 marked this pull request as draft October 13, 2022 07:59
@shani-360 shani-360 marked this pull request as ready for review October 13, 2022 08:24
@shani-360 shani-360 changed the title Update countUp.ts Update countUp.ts - support large integer numbers Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants