Skip to content

feat: transform hsl to hsla - #3850

Merged
adamwathan merged 5 commits into
tailwindlabs:masterfrom
fedeci:transform-hsl-to-hsla
Apr 30, 2021
Merged

feat: transform hsl to hsla#3850
adamwathan merged 5 commits into
tailwindlabs:masterfrom
fedeci:transform-hsl-to-hsla

Conversation

@fedeci

@fedeci fedeci commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread 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)

@fedeci fedeci Mar 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

adamwathan commented Mar 26, 2021

Copy link
Copy Markdown
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

fedeci commented Mar 26, 2021

Copy link
Copy Markdown
Contributor Author

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

@fedeci
fedeci force-pushed the transform-hsl-to-hsla branch from e6e8d4e to 7d48da0 Compare March 26, 2021 20:39
@fedeci
fedeci force-pushed the transform-hsl-to-hsla branch from 7d48da0 to 74e496b Compare March 26, 2021 20:58
@fedeci
fedeci marked this pull request as ready for review March 27, 2021 08:11
@fedeci

fedeci commented Mar 27, 2021

Copy link
Copy Markdown
Contributor Author

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

@codecov-io

codecov-io commented Mar 27, 2021 •
edited
Loading

Copy link
Copy Markdown

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

@andronocean andronocean left a comment

Copy link
Copy Markdown

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)

Comment thread __tests__/plugins/gradientColorStops.test.js Outdated
Comment thread __tests__/plugins/gradientColorStops.test.js Outdated
Comment thread __tests__/plugins/gradientColorStops.test.js Outdated
Comment thread __tests__/plugins/ringWidth.test.js Outdated
Comment thread __tests__/withAlphaVariable.test.js Outdated
andronocean
andronocean approved these changes Mar 30, 2021
View reviewed changes

@andronocean andronocean left a comment

Copy link
Copy Markdown

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

andronocean commented Apr 14, 2021

Copy link
Copy Markdown

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

adamwathan commented Apr 30, 2021

Copy link
Copy Markdown
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
@codecov-commenter

codecov-commenter commented Oct 15, 2024

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.37%. Comparing base (e227320) to head (159f493).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@andronocean andronocean andronocean approved these changes
Reviewers whose approvals may not affect merge requirements

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)

5 participants

@fedeci @adamwathan @codecov-io @andronocean @codecov-commenter