Skip to content

[color-transformers > rgbToHsbl] Fix brightness precision of HSB conversions by increasing from 2 decimal digits to 4 #3621

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 12, 2020
Merged

[color-transformers > rgbToHsbl] Fix brightness precision of HSB conversions by increasing from 2 decimal digits to 4 #3621

merged 3 commits into from
Nov 12, 2020

Conversation

@MLDimitry
Copy link
Contributor

@MLDimitry MLDimitry commented Nov 11, 2020 •
edited
Loading

WHY are these changes introduced?

I was originally investigating this issue in Shopify web https://github.com/Shopify/shopify/issues/265949

and found that the color-transformers on Polaris would convert both hex values #888888 and #878787 to the same HSB value { hue: 0, saturation: 0, brightness: 0.53 }. The calculating actually looked correct, #888888 was being converted to 0.5333 and #878787 was being converted to 0.5294. But the final step of the conversion limits these to only two decimal places, which resulted in the brightness values being rounded to 0.53 in both cases and losing their precision.

The Shopify Web ticket has more details on the original issue.

WHAT is this pull request doing?

This PR fixes the color-transformers issue by increasing the brightness value's precision from 2 decimal points to 4.

It also adds two tests for the failing conversions from the Shopify web issue for rgb(135, 135, 135) and rgb(136, 136, 136) to ensure they're both translated to the correct expected values.

Questions for reviewers

  • Is this change a bug fix? Or would we consider changing the number of decimal digits as a breaking change?
  • Thoughts on updating Polaris version in Shopify web? Should we create a workaround/short term fix in Shopify web for now instead of updating Polaris during the holiday critical phase?

🎩 checklist

@MLDimitry MLDimitry added the Bug Something is broken and not working as intended in the system. label Nov 11, 2020
@MLDimitry MLDimitry self-assigned this Nov 11, 2020
@ghost
Copy link

ghost commented Nov 11, 2020

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 11, 2020 •
edited
Loading

🟡 This pull request modifies 3 files and might impact 68 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 68)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/color-transformers.ts (total: 68)

Files potentially affected (total: 68)

🧩 src/utilities/tests/color-transformers.test.ts (total: 0)

Files potentially affected (total: 0)

@MLDimitry MLDimitry requested a review from alex-page November 11, 2020 19:54
@MLDimitry MLDimitry marked this pull request as ready for review November 11, 2020 19:55
@alex-page alex-page requested a review from kyledurand November 11, 2020 20:00
kyledurand
kyledurand approved these changes Nov 11, 2020
View reviewed changes
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

👍

@MLDimitry
Copy link
Contributor Author

MLDimitry commented Nov 11, 2020 •
edited
Loading

@alex-page @kyledurand Any thoughts on these two questions before I merge:

  • Is this change a bug fix? Or would we consider changing the number of decimal digits as a breaking change? Our type hasn't changed so I lean towards bug fix, since consumers shouldn't rely on toFixed(2) implementation details
  • Since Polaris UNRELEASED.md contains more than just bug fixes, I don't think we want to update the version in Shopify/web directly during code chill, would you agree and I should work on a separate workaround in web for now? Or should I work to cut a new release?

@kyledurand
Copy link
Member

kyledurand commented Nov 11, 2020

Is this change a bug fix? Or would we consider changing the number of decimal digits as a breaking change?

Could be considered a bug fix but I can't see it as a breaking change. If you need this available in web right away then we could cut a release with just this commit in it.

@MLDimitry
Copy link
Contributor Author

MLDimitry commented Nov 12, 2020

If you need this available in web right away then we could cut a release with just this commit in it.

Yes I think that would be best, then I can tophat the fix on web staging before bumping there.

I'm going to kick off the merge this morning. If you guys can help me cut the release once it's merged, or point me to some docs on the process so I can release it myself, that'd be great.

@MLDimitry MLDimitry merged commit ba698f1 into master Nov 12, 2020
@MLDimitry MLDimitry deleted the color-transform-fix branch November 12, 2020 14:03
@ghost
Copy link

ghost commented Nov 12, 2020

🎉 Thanks for your contribution to Polaris React!

@MLDimitry MLDimitry restored the color-transform-fix branch November 12, 2020 15:59
kyledurand pushed a commit that referenced this pull request Nov 13, 2020
@MLDimitry @kyledurand
[color-transformers > rgbToHsbl] Fix brightness precision of HSB conv…
921f1d7 
…ersions by increasing from 2 decimal digits to 4 (#3621)

* increase HSB brightness precision to 4 decimals

* use past tense
MLDimitry added a commit that referenced this pull request Nov 13, 2020
@MLDimitry
[color-transformers > rgbToHsbl] Fix brightness precision of HSB conv…
05c69ba 
…ersions by increasing from 2 decimal digits to 4 (#3621)

* increase HSB brightness precision to 4 decimals

* use past tense
@MLDimitry MLDimitry mentioned this pull request Nov 16, 2020
Merged
sylvhama pushed a commit that referenced this pull request Mar 26, 2021
@MLDimitry @sylvhama
[color-transformers > rgbToHsbl] Fix brightness precision of HSB conv…
d9b1c74 
…ersions by increasing from 2 decimal digits to 4 (#3621)

* increase HSB brightness precision to 4 decimals

* use past tense
@BPScott BPScott deleted the color-transform-fix branch May 22, 2021 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kyledurand kyledurand kyledurand approved these changes

@alex-page alex-page alex-page approved these changes

Assignees

@MLDimitry MLDimitry

Labels

Bug Something is broken and not working as intended in the system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MLDimitry @kyledurand @alex-page