Fix(adjustHue): account for potential negative hue value #438
Conversation
Due to the Javascript modulo bug (https://stackoverflow.com/questions/4467539/javascript-modulo-gives-a-negative-result-for-negative-numbers), any negative value for hue results in a bug where the resulting color will be black. Negative hues should cycle through to 359, so that functions like `adjustHue()` can provide consistent behavior for all colors. fix styled-components#437
Since the % operation is already performed inside `hslToRgb()` in `src/internalHelpers/_hslToRgb.js` (which `adjustHue()` eventually calls through `toColorString()`), this % is completely redundant. refactor styled-components#437
Codecov Report
@@ Coverage Diff @@
## master #438 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 80 80
Lines 560 560
Branches 166 166
=====================================
Hits 560 560
Continue to review full report at Codecov.
|
src/internalHelpers/_hslToRgb.js
Outdated
| @@ -22,9 +22,9 @@ function hslToRgb( | |||
| } | |||
|
|
|||
| // formular from https://en.wikipedia.org/wiki/HSL_and_HSV | |||
There was a problem hiding this comment.
Typo here we should fix as part of this.
src/internalHelpers/_hslToRgb.js
Outdated
| @@ -22,9 +22,9 @@ function hslToRgb( | |||
| } | |||
|
|
|||
| // formular from https://en.wikipedia.org/wiki/HSL_and_HSV | |||
| const huePrime = hue % 360 / 60 | |||
| const huePrime = (((hue % 360) + 360) % 360) / 60 // Corrected for potential negative hue | |||
There was a problem hiding this comment.
Instead of an inline comment, might be worth putting a note in the docs on how we handle the potential negative hue.
There was a problem hiding this comment.
I fixed the typo and removed the comment. I'll try looking into the docs later, when I have a bit more time.
Test hslToRgb's ability to handle negative hues passed in through adjustHue. tests styled-components#437
|
Just added a test to |
|
🎉 This PR is included in version 3.3.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #437