-
Notifications
You must be signed in to change notification settings - Fork 566
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
Conversation
There was a problem hiding this 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)" |
There was a problem hiding this comment.
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?
The hsla transform has better browser support, but the hsl transform uses
the CSS color module level 4 color spec syntax. It's analogous to wanting
to use a newer JavaScript syntax like Array.map or something.
https://www.w3.org/TR/css-color-4/#the-hsl-notation
…On Fri., Feb. 7, 2020, 5:41 p.m. Danny Banks, ***@***.***> wrote:
***@***.**** commented on this pull request.
Looks good to me, I just have a question on the difference of the 2
transforms.
Thank you for this addition!
------------------------------
In lib/common/transforms.js
<#383 (comment)>:
> + 'color/hsla': {
+ type: 'value',
+ matcher: isColor,
+ transformer: function (prop) {
+ return Color(prop.value).toHslString();
+ }
+ },
+
+ /**
+ * Transforms the value into an HSL string, using fourth argument if alpha is present.
+ *
+ * ```js
+ * // Matches: prop.attributes.category === 'color'
+ * // Returns:
+ * "hsl(174 100% 29%)"
+ * "hsl(174 100% 29% / .5)"
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?
------------------------------
In __tests__/common/transforms.test.js
<#383 (comment)>:
> @@ -281,6 +281,38 @@ describe('common', () => {
});
});
+ describe('color/hsl', () => {
Yes! Unit tests! 🙌
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#383?email_source=notifications&email_token=AGAOZVDR7XGFRM2KCJKSVATRBXPRFA5CNFSM4KRU4V52YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUYC56Q#pullrequestreview-355479290>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGAOZVDMJM2B2WEPXSLMSXLRBXPRFANCNFSM4KRU4V5Q>
.
|
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 |
Perhaps `hsla` and `hsl-4`?
…On Mon., Mar. 16, 2020, 7:21 p.m. Danny Banks, ***@***.***> wrote:
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?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#383 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGAOZVC4JIFU4XFSFDLAJUDRH2YBJANCNFSM4KRU4V5Q>
.
|
That makes sense to me, what do you think? |
Let's do it
…On Wed., Apr. 1, 2020, 1:02 p.m. Danny Banks, ***@***.***> wrote:
That makes sense to me, what do you think?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#383 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGAOZVHVJAW2A5WI2EO6MNDRKNXQZANCNFSM4KRU4V5Q>
.
|
LGTM |
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.
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.