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

rgbToHsb() fails on input containing { red: 255 } #251

Closed
emcmanus opened this issue Dec 29, 2017 · 3 comments
Closed

rgbToHsb() fails on input containing { red: 255 } #251

emcmanus opened this issue Dec 29, 2017 · 3 comments

Comments

@emcmanus
Copy link

emcmanus commented Dec 29, 2017

Issue summary

rgbToHsb() fails on any input containing { red: 255 }

I think the error is related to how rgbToHsb calculates hue when red is the largest color value.

Expected behavior

rgbToHsb({red: 255, green: 0, blue: 183, alpha: 1})

This should return { hue: 317, ... }, which you can calculate using http://www.workwithcolor.com/color-converter-01.htm

Actual behavior

rgbToHsb({red: 255, green: 0, blue: 183, alpha: 1})

This incorrectly returns { hue: 0, ... }

Steps to reproduce the problem

  1. import { rgbToHsb } from "@shopify/polaris";
  2. Call rgbToHsb() with a value containing { red: 255 }

Specifications

  • Polaris version: 1.9.1 (present since v1.0.1)
  • React version: N/A
  • Browser: N/A
  • Device: N/A
  • Operating System: N/A

⚓️ We’re not accepting pull requests at this time

🗒 This repo is for reporting issues and feature requests only

@emcmanus emcmanus changed the title rgbToHsb fails on any input containing { red: 255 } rgbToHsb fails on any input containing red value of 255 Dec 29, 2017
@emcmanus emcmanus changed the title rgbToHsb fails on any input containing red value of 255 rgbToHsb() fails on input containing { red: 255 } Dec 29, 2017
@emcmanus
Copy link
Author

Here's the fix e2a72fe

@emcmanus
Copy link
Author

Incidentally by rounding hue before the return you introduce precision loss that prevents a proper mapping between HSB and RGB.

@dfmcphee
Copy link
Contributor

Hey @emcmanus, thanks for the issue. This should now be fixed as of 1.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants