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(javascript-calculator): removing +0 and -0 from the expression displayed on calculator. #380

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

CallmeHongmaybe
Copy link

Checklist:

Partially closes #49045

With the 6 trailing zeros gone, I think it's still useful to remove any arithmetic involving zeros. That way the expression looks cleaner and the fix can even handle cases where the user types --0.

@CallmeHongmaybe
Copy link
Author

CallmeHongmaybe commented Feb 18, 2023

Hey maintainers,
Before we can merge, how can I tag in @spiritanand as my fellow contributor?

Copy link
Contributor

@scissorsneedfoodtoo scissorsneedfoodtoo left a comment

Choose a reason for hiding this comment

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

Hi @CallmeHongmaybe, thanks for your patience.

I took a look and it seems like this is off to a good start. However, I did notice a couple of issues.

I just took a look and this looks like a great start. While I prefer seeing the entire equation printed out after the "=" is pressed, I can see how stripping out most zeros can make things look much cleaner for things like 0+0+0+3, which now shows as 0+3.

However, when I was doing some testing, I noticed some issues with floating point arithmetic. For example, 0.2 + 0.3 throws Uncaught SyntaxError: Unexpected number.

When I add in some console logs, it looks like expression is 0.2.3 right before it gets passed to eval, which leads to the error.

Also, we could look at adding @spiritanand as a contributor manually.

But if @spiritanand can push to your branch, that would probably be easier. Or @spiritanand could also start a review and leave suggestions for possible fixes. If those suggestions are committed, then they would get credit, too.

@scissorsneedfoodtoo scissorsneedfoodtoo added bug Something isn't working blocked labels Mar 8, 2023
expression = expression
.replace(/x/g, '*')
.replace(/‑/g, '-')

Choose a reason for hiding this comment

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

You could probably remove the spaces for here and above.

Copy link
Author

Choose a reason for hiding this comment

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

Will do.
Also can you help with the tests ? Because I'm currently on a stretch.

@CallmeHongmaybe
Copy link
Author

@scissorsneedfoodtoo thanks for the edge case.

Such as floats like 0.x in expressions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JavaScript Calculator displays an incorrect formula
3 participants