[color-transformers > Hsbl] Increase precision of hue, saturation, lightness, and alpha #3640
Conversation
MLDimitry
added
the
Bug
MLDimitry
changed the title
| export function roundNumberToDecimalPlaces(value: number, decimals: number) { | ||
| // @ts-ignore - string concatenation represents exponential number notation | ||
| return Number(Math.round(value + 'e' + decimals) + 'e-' + decimals); | ||
| } |
There was a problem hiding this comment.
On my continued exploration of floating point fun, I found that toFixed(2) (which uses Math.round for it's rounding) does not accurately round all floating point numbers. i.e. 1.005 in floating point is really 1.004999995, or a value very close but below 1.005 which causes it to be rounded to 1.00 instead of 1.01. (I have a test for this case)
Here's an interesting blog post on rounding which led me to this solution which converts the numbers to exponential notation for more accurate rounding.
MLDimitry
force-pushed
the
color-transformer-moar-precision
branch
from
6a297f7 to
0a2a175
Compare
|
The 2 Percy snapshot failures look like an improvement, since video thumbnails now load, when previously they didn't? Could someone more familiar take a look? |
|
|
kyledurand
requested review from
BPScott and
kaelig
and removed request for
alex-page
kyledurand
requested review from
AndrewMusgrave and
danrosenthal
and removed request for
danrosenthal
|
I was pushing updates to get failing tests/linting/TypeScript issues fixed and they look like they all are now. This is ready for review, and if someone could also take a look at the two failing Percy screenshots (I think they can be approved but hoping for 2nd more contextful opinion), that'd be great. |
|
Bumping this for a review, hopefully y'all have some time around the Alpha Polaris launch. Thanks! |
AndrewMusgrave
left a comment
AndrewMusgrave
left a comment
There was a problem hiding this comment.
Thanks for the articles! Makes sense to me, nice job 💯 You'll want someone from the new design language to take a look as well though
BPScott
left a comment
BPScott
left a comment
There was a problem hiding this comment.
Great sleuthing!
Two little requests for comment adjusting, then we're good to go.
Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>
Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>
|
Thanks for the comments, reviews, and suggestions. I've applied them and resolved new conflicts. This is ready for another review when folks can. |
…ghtness, and alpha (#3640) * update float precision of hue, saturation, lightness, and alpha * add UNRELEASED.md entry * add roundNumberToDecimalPlaces for accurate rounding * improved hsbla rounding in color-transformers * push eslint ignore * make rounding function more readable/fix eslint + ts * remove trivially inferred type annotations * more rounding of values before being stringified * update failing tests * Update UNRELEASED.md Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com> * Update comment in src/utilities/roundNumberToDecimalPlaces.ts Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com> Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>
…ghtness, and alpha (#3640) * update float precision of hue, saturation, lightness, and alpha * add UNRELEASED.md entry * add roundNumberToDecimalPlaces for accurate rounding * improved hsbla rounding in color-transformers * push eslint ignore * make rounding function more readable/fix eslint + ts * remove trivially inferred type annotations * more rounding of values before being stringified * update failing tests * Update UNRELEASED.md Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com> * Update comment in src/utilities/roundNumberToDecimalPlaces.ts Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com> Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>
3 participants
WHY are these changes introduced?
Fixes https://github.com/Shopify/shopify/issues/265949
I originally solved part of this problem by increasing the precision of brightness to 4 decimal places in #3621
What I realized testing my solution in production was I had missed the fact that other colour values would also lose precision in their hue, saturation, lightness, and alpha. This PR increases the precision of the other fields and also adds a function for more accurate rounding to decimals.
Before - Note color gets replaced after pasting
After - Note color doesn't get replaced
WHAT is this pull request doing?
This PR improves the fix for color-transformers by also increasing precision of other HSBLA values.