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

Rgba #62

Merged
merged 5 commits into from
Jan 14, 2017
Merged

Rgba #62

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .documentation.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"description": "Functions that help creating and modifying colors."
},
"rgb",
"rgba",
{
"name": "Shorthands",
"description": "Functions that help write nicer code. They're shorter, of course, but they also make it easy to pass in multiple variables without resorting to string concatenation."
Expand Down
190 changes: 159 additions & 31 deletions docs/docs/index.html

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/helpers/rgb.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type RgbColor = {
}

/**
* Returns a string value for the color. The returned result is the smalles possible hex notation.
* Returns a string value for the color. The returned result is the smallest possible hex notation.
*
* @example
* // Styles as object usage
Expand Down
53 changes: 53 additions & 0 deletions src/helpers/rgba.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// @flow

import rgb from './rgb'

type RgbaColor = {
red: number,
green: number,
blue: number,
alpha: number,
}

/**
* Returns a string value for the color. The returned result is the smallest possible rgba or hex notation.
*
* @example
* // Styles as object usage
* const styles = {
* background: rgba(255, 205, 100, 180),
* background: rgba({ red: 255, green: 205, blue: 100, alpha: 180 }),
* background: rgba(255, 205, 100, 255),
* }
*
* // styled-components usage
* const div = styled.div`
* background: ${rgba(255, 205, 100, 180)};
* background: ${rgba({ red: 255, green: 205, blue: 100, alpha: 180 })};
* background: ${rgba(255, 205, 100, 255)};
* `
*
* // CSS in JS Output
*
* element {
* background: "rgba(255,205,100,180)";
* background: "rgba(255,205,100,180)";
* background: "#ffcd64";
* }
*/
function rgba(value: RgbaColor | number, green?: number, blue?: number, alpha?: number): string {
if (typeof value === 'number' &&
typeof green === 'number' &&
typeof blue === 'number' &&
typeof alpha === 'number') {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't require alpha imo, this should work perfectly fine (I think right now it'd throw an error):

rgba(255, 205, 100)

Copy link
Member Author

Choose a reason for hiding this comment

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

@mxstbr this was on purpose as it doesn't work in Chrome nor Firefox

screen shot 2016-12-25 at 23 07 04

screen shot 2016-12-25 at 23 09 13

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we make it work? if alpha == null rgb(value, green, blue)

Much nicer experience, especially if used programmatically?

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

  1. If we set alpha to 1, should we also have default values for red, green, blue? Is it 0 or 255 or another value?
  2. Should we check that red, green, blue are within the 0 - 255 boundary and alpha between 0 - 1?

Thoughts?

Copy link
Contributor

@bhough bhough Dec 31, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Let's always throw an error – rgba(255, 255, 255) in CSS doesn't work either!

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
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, should also be the case with alpha === undefined|null?

? rgb(value.red, value.green, value.blue)
: `rgba(${value.red},${value.green},${value.blue},${value.alpha})`
}

throw new Error('Passed invalid arguments to rgba, please pass multiple numbers e.g. rgb(255, 205, 100, 180) or an object e.g. rgb({ red: 255, green: 205, blue: 100, alpha: 180 }).')
}

export default rgba
29 changes: 29 additions & 0 deletions src/helpers/test/__snapshots__/rgba.test.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
exports[`rgb should convert a rgba object to a rgba string 1`] = `
Object {
"background": "rgba(255,205,100,180)",
}
`;

exports[`rgb should convert a rgba object with full opacity to a hex color 1`] = `
Object {
"background": "#ffcd64",
}
`;

exports[`rgb should convert a rgba object with full opacity to a reduced hex color 1`] = `
Object {
"background": "#fff",
}
`;

exports[`rgb should convert multiple numbers to a rgba string 1`] = `
Object {
"background": "rgba(255,205,100,180)",
}
`;

exports[`rgb should convert multiple numbers with full opacity to a hex color 1`] = `
Object {
"background": "#ffcd64",
}
`;
29 changes: 29 additions & 0 deletions src/helpers/test/rgba.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// @flow
import rgba from '../rgba'

describe('rgb', () => {
it('should convert multiple numbers to a rgba string', () => {
expect({ background: rgba(255, 205, 100, 180) }).toMatchSnapshot()
})

it('should convert multiple numbers with full opacity to a hex color', () => {
expect({ background: rgba(255, 205, 100, 255) }).toMatchSnapshot()
})

it('should convert a rgba object to a rgba string', () => {
expect({ background: rgba({ red: 255, green: 205, blue: 100, alpha: 180 }) }).toMatchSnapshot()
})

it('should convert a rgba object with full opacity to a hex color', () => {
expect({ background: rgba({ red: 255, green: 205, blue: 100, alpha: 255 }) }).toMatchSnapshot()
})

it('should convert a rgba object with full opacity to a reduced hex color', () => {
expect({ background: rgba({ red: 255, green: 255, blue: 255, alpha: 255 }) }).toMatchSnapshot()
})

it('should throw an error if an object and multiple arguments are passed', () => {
expect(() => ({ background: rgba({ red: 255, green: 1, blue: 1, alpha: 180 }, 250, 100) }))
.toThrow('Passed invalid arguments to rgba, please pass multiple numbers e.g. rgb(255, 205, 100, 180) or an object e.g. rgb({ red: 255, green: 205, blue: 100, alpha: 180 }).')
})
})
2 changes: 2 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import wordWrap from './mixins/wordWrap'

// Color
import rgb from './helpers/rgb'
import rgba from './helpers/rgba'

// Shorthands
import animation from './shorthands/animation'
Expand Down Expand Up @@ -62,6 +63,7 @@ export {
rem,
retinaImage,
rgb,
rgba,
selection,
size,
stripUnit,
Expand Down