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

Tint and Shade Helpers #100

Merged
merged 4 commits into from Mar 5, 2017
Merged

Tint and Shade Helpers #100

merged 4 commits into from Mar 5, 2017

Conversation

bhough
Copy link
Contributor

@bhough bhough commented Mar 3, 2017

Adds tint and shade helpers. Also fixes some issues with documentation and alphabetizes color imports.

Addresses items from #1.

Adds tint and shade helpers. Also fixes some issues with documentation and alphabetizes color

imports.

Addresses item from #1
@mxstbr mxstbr mentioned this pull request Mar 3, 2017
50 tasks
@codecov
Copy link

codecov bot commented Mar 3, 2017

Codecov Report

Merging #100 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #100   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          55     57    +2     
  Lines         324    332    +8     
=====================================
+ Hits          324    332    +8
Impacted Files Coverage Δ
src/color/mix.js 100% <ø> (ø)
src/color/tint.js 100% <100%> (ø)
src/color/shade.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 325cbce...7c68902. Read the comment docs.

@bhough bhough requested review from nikgraf and mxstbr March 3, 2017 22:37
@nikgraf
Copy link
Member

nikgraf commented Mar 3, 2017

Is tint & shade the same as darken/lighten or different? I find it bit confusing:)

@bhough
Copy link
Contributor Author

bhough commented Mar 3, 2017

Best explanation I've read:

Tint and shade are effectively mixing with white and black respectively, whereas lighten/darken are manuipulating the luminance channel independently of hue and saturation. The former can produce hue shifts, whereas the latter does not.


function shade(percentage: number, color: string) {
if (!percentage || !color) throw new Error('Passed insufficient arguments to shade, please pass a percentage and a color to be shaded.')
if (typeof percentage !== 'number' || percentage > 1) throw new Error('Passed an incorrect argument to shade, please pass a percentage less than or equal to 1.')
Copy link
Member

Choose a reason for hiding this comment

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

Should we check for a range between -1 to 1? -1 would basically tint the color if I'm not mistaken, but still a valid input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to update this to capture the negative use case, but I'm not sure I like the idea of allowing either helper to do the job of the other by accident (putting in a negative number). It feels like it would be better to limit it based on each helpers use case. Thoughts?

import mix from './mix'

/**
* Shades a color by mixing it with black. Leverages the mix color module.
Copy link
Member

Choose a reason for hiding this comment

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

@bhough can you remove Leverages the mix color module? I think it doesn't add much value for a user, but I really liked your explanation in the comment. What if we do something like this:

Shades a color by mixing it with black. Compared to `darken` it can produce hue shifts, wheres `darken` manipulates the luminance channel and therefor doesn't produce hue shifts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that, I'll update it.

function shade(percentage: number, color: string) {
if (!percentage || !color) throw new Error('Passed insufficient arguments to shade, please pass a percentage and a color to be shaded.')
if (typeof percentage !== 'number' || percentage > 1) throw new Error('Passed an incorrect argument to shade, please pass a percentage less than or equal to 1.')
if (typeof color !== 'string') throw new Error('Passed an incorrect argument to shade, please pass a string representation of a color.')
Copy link
Member

Choose a reason for hiding this comment

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

mix is checking if it's a color valid to parse. We can drop this check 😄


function tint(percentage: number, color: string) {
if (!percentage || !color) throw new Error('Passed insufficient arguments to tint, please pass a percentage and a color to be tinted.')
if (typeof percentage !== 'number' || percentage > 1) throw new Error('Passed an incorrect argument to tint, please pass a percentage less than or equal to 1.')
Copy link
Member

Choose a reason for hiding this comment

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

See shade comments

@nikgraf
Copy link
Member

nikgraf commented Mar 5, 2017

@bhough let's merge this PR. I will be on a plane for 5h and clean it up there. Just want to avoid merge conflicts later on 😄

@nikgraf nikgraf merged commit 6542b6d into master Mar 5, 2017
@nikgraf nikgraf deleted the tint-shade branch March 5, 2017 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants