Closed nikgraf closed 7 years ago
I love this!!
Two discussion points:
I think we should accept the object structure too, some people prefer that and this shouldn't be the most opinionated lib – it's not like that hurts us. (also, jss
has added support for object syntax for almost everything so people might be passing those in as variables)
I like the fp/ramda-style design! I don't think we should necessarily sell it like that, but even if beginner users use it (and don't even compose
) compare these two:
// Non-fp style
saturate(darken(rgb(0, 100, 0), 10, 20)
// Fp/ramda style
saturate(20, darken(10, rgb(0, 100, 0)));
The second one is so much easier to read, because you avoid having to backtrack with your eyes constantly. On top of that you can compose functions which is really powerful, so I think it'd be a good idea to build it like that even though it deviates from the Sass API a bit.
What do you think about that @bhough?
Just out of interest, would it be better to have fewer top-level functions and instead return a smarter object?
const baseBlue = rgb(0, 0, 150)
const X = styled.div`
background: ${baseBlue};
border: 2px solid ${baseBlue.darken(20).saturate(10)};
&:hover { background: ${baseBlue.lighten(10)} }
`
We could do this by either overloading toString
or defining something like toCSS()
that styled-components knows about when doing the interpolations. Feels like a better way of encapsultating the potential colour-related operations than a bunch of top-level methods.
color
does this, but I'm not a fan. It's pretty tedious to use and is totally different from Bourbon/Sass (which most people will know) so the learning curve will be pretty high, which I'd like to avoid.
Having separate functions seems much more sensible to me, things can always be extracted to variables or functions.
Hmm, what makes it tedious? Looks pretty ok to me, just the hsl().string()
is a bit superfluous.
I don't think trying to make it like bourbon and Sass is all that important. JS is different, we have real objects with methods, not just strings.
Global functions would be great, though, if you could do this:
const base = '#444';
styled.div`
background: ${base};
border: ${darken(20, base)};
`
But yeah, IMO if you have to start by wrapping colour values in an object then you may as well use object methods to manipulate it.
@geelen Spot on. This is in my initial proposal as well, but not good enough explained. Just updated the docs.
I would recommend: Functions like saturate accept hex strings or rgb or rgba or hsla strings e.g. '#FFDDFF' or 'rgb(22, 255, 0)'
If I understood correctly Max would like to support both: the function and the string notation. The object notation in the end also returns a hex string e.g. '#FFF' === rgb({ red: 255, green: 255, blue: 255 })
darken(20, '#444');
darken(20, rgb({ red: 100, green: 100, blue: 100 }));
@mxstbr totally agree, I wouldn't mention Rambda or FP at all in the docs. Just an example on how to compose it. People who like FP will notice it anyway …
So everybody happy with the API? We do object and string notation?
Sorry a little late to the discussion here:
I think we should accept the object structure too, some people prefer that and this shouldn't be the most opinionated lib – it's not like that hurts us.
Definitely agree that we should support object notation wherever possible. The goal is to make polished work well with any css as js approach, and that will go a long way towards that.
Having separate functions seems much more sensible to me, things can always be extracted to variables or functions.
As I think I mentioned in a previous issue, the problem with forcing colors to be instances of a color class of sorts, is that every color needs to be setup that way from the start. That is hard to ask of users I believe. I wouldn't want to force people using polished with a theme that wasn't built withpolished
to have to convert all colors over to our proprietary color implementation just to be able to manipulate them.
As @mxstbr mentioned, that was one of the reasons we shied away from using color
.
@bhough what do you think about the API I described + support for object notation? Anything to change?
Stick to supporting the CSS spec for modifying colors, or else this will be much like the dark days of using mixins for vendor prefixes. You had to remember when to use them for your level of browser support, and the refactoring when Autoprefixer came on the scene was a hassle.
Ideally when browser support is good, transforming the color function values can be dropped without users having to refactor their style rules. A lot of teams are already used to using the spec way to manipulate colors thanks to cssnext.
Here is the postcss color function plugin: postcss/postcss-color-function.
@jaydenseric awesome suggestion, didn't know about it
@jaydenseric I learned that a lot of these support percentage as a value from 0-100 if a percent sign %
is used. We can't do this in JS, but used 0-1 instead. This also matches with the alpha definition. Do you have any other suggestion?
If it were a JS API (as opposed to parsing the CSS strings in place) that would be fine, along with separating parameters with commas. The most useful thing from the spec would be to copy function names and behaviors.
At that point, it seems better for advanced users to install their own color library of choice that allows destructured importing of just the APIs they need. Color libraries can massively bloat client bundles. Often by added one more variable to your theme palette you can remove the need for a 30kb library.
43 components into my current styled-components project (with a theme configured with a 5 color palette) and the only 2 color APIs that would be nice are hex -> rgba, and to a lesser extent perhaps brightening/darkening.
Here is my tiny helper:
/**
* Converts a CSS hex color value to RGBA.
* @param {string} hex - Expanded hexadecimal CSS color value.
* @param {number} alpha - Alpha as a decimal.
* @returns {string} RGBA CSS color value.
*/
export function hex2Rgba (hex, alpha) {
const r = parseInt(hex.substring(1, 3), 16)
const g = parseInt(hex.substring(3, 5), 16)
const b = parseInt(hex.substring(5, 7), 16)
return `rgba(${r}, ${g}, ${b}, ${alpha})`
}
@nikgraf anything remaining from this issue we want to address or is this good to close?
@bhough good to close 👍
hey, @nikgraf + @bhough, thanks for polished
! @nikgraf, I'm looking to better understand color theory. Are you able to share some resources that were most helpful to you?
I told @mxstbr to help out here and especially with the color related API 🙌 Before starting to code I want figure out how you think about the API. There was already some discussion here https://github.com/styled-components/polished/pull/24, but there are a couple things unclear to me. This is a proposal. Please correct me if I'm wrong or you see room for improvement.
#FF553A
. If possible only 3 characters#FFF
instead of#FFFFFF
. In case of opacity we generatergba(255,0,0,0.3)
.rgb('22, 255, 0')
andrgb(22, 255, 0)
andrgb({ red: 22, green: 255, blue: 0 })
. Not sure about the object structure. Looks handy, but usually I prefer to have one recommended way of doing things and by exposing less possible ways we would enforce it. (We can always add it later)saturate
accept hex strings or rgb or rgba or hsla strings e.g.'#FFDDFF'
or'rgb(22, 255, 0)'
, but don't accept plain single values like rgb (there is no conversion). Otherwise it could get confusion if they accept hsla or rgba. On the other hand if we could accept the object notation, but as said it's not necessary.Note: please ignore 10 & 20 arguments. I need to learn more about color theory first to understand what parameters make sense.
What do you guys think? @bhough @mxstbr @guzart