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

Jump to bottom

feat: transform hsl to hsla #3850

Merged
merged 5 commits into from Apr 30, 2021
Merged

feat: transform hsl to hsla #3850

merged 5 commits into from Apr 30, 2021

Conversation

fedeci
Copy link
Contributor

@fedeci fedeci commented Mar 26, 2021

This fixes #3834
We preserve the hsl colors by converting it to hsla instead of rgba.
e.g.

hsl(0, 0%, 0%)
- rgba(0, 0, 0, var(--opacity))
+ hsla(0, 0, 0, var(--opacity))

fedeci
fedeci commented Mar 26, 2021
View reviewed changes
src/util/withAlphaVariable.js
const [r, g, b, a] = toRgba(color)
const isHSL = color.startsWith('hsl')

const [i, j, k, a] = isHSL ? toHsla(color) : toRgba(color)
Copy link
Contributor Author

@fedeci fedeci Mar 26, 2021 •
edited

Choose a reason for hiding this comment

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

We use generic component names because we don't know if it will be rgb or hsl

@adamwathan
Copy link
Member

Looking good, I noticed you still have this set as a draft so just let me know when you'd like me to look into merging 👍🏻 Thanks!

@fedeci
Copy link
Contributor Author

fedeci commented Mar 26, 2021

Yess, I saw that toRgba is used in other places, so I would take a look at those too.

@fedeci fedeci marked this pull request as ready for review March 27, 2021 08:11
@fedeci
Copy link
Contributor Author

fedeci commented Mar 27, 2021

@adamwathan it should be ready to be merged if it passes the tests

@codecov-io
Copy link

codecov-io commented Mar 27, 2021 •
edited

Codecov Report

Merging #3850 (159f493) into master (e227320) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3850      +/-   ##
==========================================
+ Coverage   93.34%   93.37%   +0.02%     
==========================================
  Files         178      178              
  Lines        1849     1857       +8     
  Branches      332      342      +10     
==========================================
+ Hits         1726     1734       +8     
  Misses        105      105              
  Partials       18       18
Impacted Files Coverage Δ
src/plugins/gradientColorStops.js 100.00% <100.00%> (ø)
src/plugins/ringWidth.js 100.00% <100.00%> (ø)
src/util/withAlphaVariable.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e227320...159f493. Read the comment docs.

andronocean
andronocean suggested changes Mar 27, 2021
View reviewed changes
Copy link

@andronocean andronocean left a comment

Choose a reason for hiding this comment

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

This isn't quite right. The saturation and lightness parameters in hsl() or hsla() colors are percentages, not numbers, so they need to be followed by the % symbol. The tests pass, but the tests are expecting the wrong values. (See MDN description)

__tests__/plugins/gradientColorStops.test.js Outdated Show resolved Hide resolved
__tests__/plugins/gradientColorStops.test.js Outdated Show resolved Hide resolved
__tests__/plugins/gradientColorStops.test.js Outdated Show resolved Hide resolved
__tests__/plugins/ringWidth.test.js Outdated Show resolved Hide resolved
__tests__/withAlphaVariable.test.js Outdated Show resolved Hide resolved
andronocean
andronocean approved these changes Mar 30, 2021
View reviewed changes
Copy link

@andronocean andronocean left a comment

Choose a reason for hiding this comment

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

Looks good to me 😎 (Thank you @fedeci !)

@andronocean
Copy link

Hey @adamwathan, any way to get this merged this week or next? It'd be really helpful 🙏

@adamwathan adamwathan merged commit 92bb81e into tailwindlabs:master Apr 30, 2021
@adamwathan
Copy link
Member

Thanks! Will try get this out in a patch soon.

adamwathan pushed a commit that referenced this pull request May 7, 2021
@fedeci @adamwathan
feat: transform hsl to hsla (#3850)
4daf57e 
* feat: transform `hsl` to `hsla`

* feat: update plugins using `toRgba`

* Test `gradientColorStops`

* Add test for `ringWidth`

* Add percentage symbol after Saturation and Lightness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andronocean andronocean andronocean approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Preserve HSL colors instead of converting to RGBA (keeps colors on older browsers)
4 participants
@fedeci @adamwathan @codecov-io @andronocean