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: inaccurate floating-point calculations #1369

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

Conversation

bilal-auz
Copy link

What does it do?

I used the big.js library to overcome the floating-point error that occurs in the NumberInput component when using the increment and decrement functions. The new implementation ensures that arithmetic operations are accurately performed, especially when dealing with decimal numbers.
image

Why is it needed?

These changes address the issues raised in Strapi/Strapi repo issue#18138.
In the main project, when trying to fill the restaurant fields, a bug occurs when selecting an averagePrice of 0.06 using the arrow controls in the restaurant fields. The arrows provide inaccurate precision, resulting in 0.0600000001 instead of 0.06 when reaching this value. This issue also randomly affects other values.
See the issue Strapi/Strapi repo issue#18138 for better understanding

The issue demonstration:-

AveragePrice-Bug

How to test it?

I haven't completed the test yet due to the discovery of some missing testing packages, which can be seen as another issue. As a result, I've chosen to divide the task into two separate steps.

Related issue(s)/PR(s)

The related issue is mentioned in the main repo of strapi: strapi/strapi#18138

@changeset-bot
Copy link

changeset-bot bot commented Sep 26, 2023

⚠️ No Changeset found

Latest commit: eb5b7f8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Sep 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
design-system ❌ Failed (Inspect) Sep 27, 2023 2:01pm
design-system-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2023 2:01pm

@amerikan
Copy link
Contributor

By the way, the way the native input[type=number] handles steps is to just round up to whole number, then increment: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number#step

@joshuaellis
Copy link
Member

Why do we need to use a library to handle this? We already have a library for number handling, im not sure I see any benefit in using two, also can you resolve the conflicts please so we can at least run the CI 👍🏼

@joshuaellis joshuaellis self-assigned this Sep 27, 2023
@joshuaellis joshuaellis self-requested a review September 27, 2023 06:55
@joshuaellis joshuaellis added pr: fix This PR is fixing a bug source: design-system relates to design-system package labels Sep 27, 2023
@bilal-auz
Copy link
Author

Why do we need to use a library to handle this? We already have a library for number handling, im not sure I see any benefit in using two, also can you resolve the conflicts please so we can at least run the CI 👍🏼

I wasn't aware that Strapi has a number handling library for mitigating floating-point issues.
Could you please provide me with more details about it?"

@joshuaellis
Copy link
Member

number handling library for mitigating floating-point issues

I didn't say this exactly, I was proposing that the library in the DS currently (look at the NumberInput) component might be able to handle what you're trying to solve.

@bilal-auz
Copy link
Author

I didn't say this exactly, I was proposing that the library in the DS currently (look at the NumberInput) component might be able to handle what you're trying to solve.

Oh, I see. I believe it's a small bug that only affects the component (NumberInput) when using the arrowUp/arrowDown functionality with a step value of 0.01. You can find more details about this problem in the linked issue here: Issue Link.

I've attempted to address this issue without relying on any external libraries, but I'm encountering a persistent floating-point problem. Additionally, I'm uncertain if using something like ".toFixed(2)" would provide a definitive solution, especially in scenarios where users input numbers with more than two decimal places.

I'd appreciate your thoughts on this matter. Please let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: design-system relates to design-system package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants