[WIP] feat(color): Named color support for color mixins. #155
Conversation
bhough
force-pushed
the
named-colors
branch
3 times, most recently
from
0e1185d
to
7edd5ab
Compare
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 63 64 +1
Lines 336 343 +7
Branches 92 96 +4
=====================================
+ Hits 336 343 +7
Continue to review full report at Codecov.
|
|
@mxstbr @nikgraf @suchipi I'm of two minds on this one: It is really nice to just be able to use named colors in all our color methods without any extra effort. But will they be used frequently enough to warrant the ~4kb (1.5 gzipped) overhead on every color mixin? Would it be better to just expose |
|
You could save some size and duplication by changing parseToRgb, parseToHsl, etc to all call nameToHex internally. Then you wouldn't need to repeat the call in each helper (though it's a little more coupled organizationally). I personally prefer the helpers handling named colors out of the box because that behavior matches what sass and stylus helpers do. When I ran into this, it was surprising to me to find this wasn't supported by polished out of the box, because someone had described polished to me as "common sass helpers for js". Exporting nameToHex (instead of calling it internally) would provide the tools necessary to handle named colors, but handling it internally provides a smoother developer experience for those who need it, since they don't have to go through the roundabout of realizing they need to handle this case and adding the helper (if they do). So to me it seems like a question of smoother DX ("pit of success") or smaller bundle size. |
There was a problem hiding this comment.
I initially didn't add a color map as I was concerned about the total output. I'm still a bit, but definitely can see the benefit for development. (I personally use them a lot for prototyping)
Let's remove the duplicated nameToHex parsings and make it happen 😄
src/color/opacify.js
Outdated
| @@ -32,7 +33,8 @@ import curry from '../internalHelpers/_curry' | |||
| * } | |||
| */ | |||
| function opacify(amount: number, color: string) { | |||
| const parsedColor = parseToRgb(color) | |||
| const normalizedColor = nameToHex(color) | |||
There was a problem hiding this comment.
@bhough I think we can drop nameToHex here since it's already done by default in parseToRgb. Same goes for all the other functions including the ones using parseToHsl
|
Good catch @nikgraf. I'll get that taken care of at some point before the weekend. |
bhough
force-pushed
the
named-colors
branch
2 times, most recently
from
f2f0610
to
99f53e7
Compare
|
@nikgraf this is ready to re-review. |
src/color/grayscale.js
Outdated
| return toColorString({ | ||
| ...parseToHsl(color), |
There was a problem hiding this comment.
@bhough one nitpick … can you revert that change? No need to assign the hslColor here
src/color/parseToHsl.js
Outdated
| // Note: At a later stage we can optimize this function as right now a hsl | ||
| // color would be parsed converted to rgb values and converted back to hsl. | ||
| return rgbToHsl(parseToRgb(color)) | ||
| return rgbToHsl(parseToRgb(normalizedColor)) |
There was a problem hiding this comment.
no need for an assignment here, best to just inline it
src/color/setSaturation.js
Outdated
| return toColorString({ | ||
| ...parseToHsl(color), | ||
| ...hslColor, |
There was a problem hiding this comment.
same here, inlining it should save us a couple bites again
There was a problem hiding this comment.
ok I originally did this because we were doing it two different ways across color modules, I'll have to change this back as well as the update the ones where we were doing an assignment.
bhough
force-pushed
the
named-colors
branch
2 times, most recently
from
ca4d5d3
to
e319cae
Compare
Adds support for CSS named colors in all color mixins. Also fixes some outdated tests for mix/shade/tint and updates associated docs.
|
@nikgraf ok third time is a charm. I actually cleaned it up some more, found a few more things that eventually call |
Adds support for CSS named colors in all color mixins. Also fixes shade and tint docs.
Addresses request in #153