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

feat: adding color/hsl and color/hsl-4 transforms #383

Merged
merged 1 commit into from
Apr 24, 2020
Merged

feat: adding color/hsl and color/hsl-4 transforms #383

merged 1 commit into from
Apr 24, 2020

Conversation

cssinate
Copy link
Contributor

@cssinate cssinate commented Feb 7, 2020

Issue #, if available:

Description of changes:
Adding in support for color transforms to color/hsla (going to to 3-argument hsl format if no alpha change) and color/hsl with newer syntax, accepting 4 arguments in the same function.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Member

@dbanksdesign dbanksdesign 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, I just have a question on the difference of the 2 transforms.

Thank you for this addition!

* // Matches: prop.attributes.category === 'color'
* // Returns:
* "hsl(174 100% 29%)"
* "hsl(174 100% 29% / .5)"
Copy link
Member

Choose a reason for hiding this comment

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

This might be a dumb question, what is the difference in the formatting of this output versus the 'color/hsla' transform? Do both work and is one preferable over the other? Both transforms seem to handle alpha and both output hsl(), do we need both?

__tests__/common/transforms.test.js Outdated Show resolved Hide resolved
@cssinate
Copy link
Contributor Author

cssinate commented Feb 7, 2020 via email

@dbanksdesign
Copy link
Member

Sorry just getting back in the swing of things. I was wondering since both transforms handle transparency and the only difference is which CSS color module spec syntax they use, would it make sense to rename the transforms to something like color/hsl and color/hsl-4 or something?

@cssinate
Copy link
Contributor Author

cssinate commented Mar 16, 2020 via email

@dbanksdesign
Copy link
Member

That makes sense to me, what do you think?

@cssinate
Copy link
Contributor Author

cssinate commented Apr 1, 2020 via email

@cssinate cssinate changed the title feat: adding color/hsl and color/hsla transforms feat: adding color/hsl and color/hsl-4 transforms Apr 14, 2020
@chazzmoney
Copy link
Collaborator

LGTM :shipit:

@dbanksdesign dbanksdesign merged commit b777cfb into amzn:master Apr 24, 2020
dbanksdesign pushed a commit that referenced this pull request Oct 5, 2020
Only difference between the two is hsl-4 uses the new hsl syntax (CSS color module level 4 color spec syntax). Both transforms handle colors with alpha transparency.
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

Successfully merging this pull request may close these issues.

3 participants