hoist-react icon indicating copy to clipboard operation
hoist-react copied to clipboard

Fix to the zeroPad not working in cases of rounding, especially due to floating point arithmetic error

Open jskupsik opened this issue 1 year ago • 1 comments

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be checked off to indicate they have been considered. If unclear if a step is relevant, please leave unchecked and note in comments.

  • [x] Caught up with develop branch as of last change.
  • [x] Added CHANGELOG entry, or determined not required.
  • [x] Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • [x] Updated doc comments / prop-types, or determined not required.
  • [x] Reviewed and tested on Mobile, or determined not required.
  • [x] Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to collapse multiple intermediate commits into a single commit representing the overall feature change. This helps keep the commit log clean and easy to scan across releases. PRs containing a single commit should be rebased when possible.

jskupsik avatar Aug 10 '24 00:08 jskupsik

I found a fairly-comprehensive list of all of the various techniques for "rounding js float numbers" to avoid the arithmetic error: https://stackoverflow.com/questions/11832914/how-to-round-to-at-most-2-decimal-places-if-necessary/48764436#48764436

I've ended up using the lodash _.round() function due to its simplicity, speed (faster than my rounding code), and the fact we have an upper bound of precision anyways.

jskupsik avatar Aug 12 '24 18:08 jskupsik

Looked again and moving ahead - this is a solid change and again it is limited only to the newer usage of zeroPad as number.

amcclain avatar Aug 30 '24 20:08 amcclain