Rgba #62
Rgba #62
Conversation
| if (typeof value === 'number' && | ||
| typeof green === 'number' && | ||
| typeof blue === 'number' && | ||
| typeof alpha === 'number') { |
There was a problem hiding this comment.
Shouldn't require alpha imo, this should work perfectly fine (I think right now it'd throw an error):
rgba(255, 205, 100)There was a problem hiding this comment.
There was a problem hiding this comment.
Why don't we make it work? if alpha == null rgb(value, green, blue)
Much nicer experience, especially if used programmatically?
There was a problem hiding this comment.
From experience null or undefined often happen due a mistake. If we make alpha optional than the static analysis via Flowtype or TypeScript can't detect these errors and it would simply be opacity 1. Which also led me to another issue: what's the correct default? 1 or 0? It probably depends on the application.
On the other hand I'm a big fan of the robustness principle: "Be conservative in what you do, be liberal in what you accept from others" https://en.wikipedia.org/wiki/Robustness_principle
2 related questions:
- If we set alpha to 1, should we also have default values for red, green, blue? Is it 0 or 255 or another value?
- Should we check that red, green, blue are within the 0 - 255 boundary and alpha between 0 - 1?
Thoughts?
There was a problem hiding this comment.
I think this is a perfect place that we can eventual leverage #4. Throw a warning and fall back to rgb if alpha isn't provided.
For alpha I think the proper fallback is 1. A fully transparent color in rgba tends to only be used in transition animations when going to or from a fully transparent color to a more opaque color.
As far as defaults for the colors, this is a tricky one. rgba(244) could be shorthand for one of rgba(244, 244, 244), rgba(244, 0, 0), or rgba(244, 255, 255). The last option makes little sense to me as omitting a color probably does not mean you want it at 100%. The first option, while jiving with other shorthands somewhat (like padding(5px)), may not be how most think about color. I think if we are going to have a default (which I believe we should), 0 makes the most sense.
I've been thinking a lot about this last question as I work on #4. My thoughts, and I would love everyone's opinion here, is that we should limit our internal error handling to those specific to the methods we define, offloading the rest to the browser.
For example timingFunctions has a set of timingFunctions, if you provide the name of one that doesn't exist we should throw an error as it is critical to how the mixin works.
However, if I pass a bad backgroundSize to retinaImage (like say a typo in the keyword), we should let that bubble up via the browser. Otherwise we will paint ourselves into a corner where we have to maintain error checking for general CSS. Did you provide this mixin bad CSS?, did you use an unsupported color keyword?. Libraries like Bourbon (or SASS in general) let you pass bad CSS into mixins, we should as well.
There was a problem hiding this comment.
Sound reasonable to me to let errors bubble up via the browser. It's a helper library and not validating all of your CSS 👍
I tried to think about when I used rgba or how I would use rgba without providing the alpha. Especially in transition animations as @bhough mentioned it never would be null or undefined and if it would, it would be a bug in my calculations.
That's what I'm mostly worried about. If we default back to 1, people might miss bugs. Can you think of an transition animation calculation where alpha would be null or undefined?
There was a problem hiding this comment.
I think @nikgraf is right here, that would only ever happen if it was a bug so it seems reasonable to not let that happen?
There was a problem hiding this comment.
yeah I was more making the case for if we wanted to provide a default for when it wasn't provided that 1 made more sense than 0. In that case it still is a question of defaulting to rgb and throwing a warning, or throwing an error.
There was a problem hiding this comment.
Let's always throw an error – rgba(255, 255, 255) in CSS doesn't work either!
| typeof alpha === 'number') { | ||
| return alpha === 255 ? rgb(value, green, blue) : `rgba(${value},${green},${blue},${alpha})` | ||
| } else if (typeof value === 'object' && green === undefined && blue === undefined && alpha === undefined) { | ||
| return value.alpha === 255 |
There was a problem hiding this comment.
Same as above, should also be the case with alpha === undefined|null?
Current coverage is 100% (diff: 100%)
|
No description provided.