Skip to content
This repository has been archived by the owner on Nov 4, 2022. It is now read-only.

No grade bug fix #58

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

diego-codes
Copy link
Contributor

@diego-codes diego-codes commented Feb 22, 2017

Closes #53

NOTE: This is a rebased PR from my original PR for IBM-Design/color-bee. I tried to be as thorough with my rebase to make sure it matches this project's structure. But there is chance, I might have missed something.

Problem

The assumption is that if I use the color function without a grade argument, I should get the core grade of the color I am requesting:

.foo {
  color: color('blue'); // Core grade is 50, so the color to return should be #2d74da
}

The default value for the $grade parameter in the color() function is set to 0 which currently no color has this as a possible grade. Updating the default $grade value to a number that does in fact exist in all palettes doesn't work because each color has a different core grade associated with it.

Solution

The solution I have come up with is to add one more grade item to each color map in the output _bee-palette.scss with the key of core and value of whatever that palette's core grade hex value is. This way we can set the default value for $grade in the color() function to 'core' and fulfill the expected behavior of calling the color() function without supplying a grade.

Along with this solutions, I took the liberty to:

  • Update the .ase, .json, and .scl output templates to include the core grade value.
  • Improve (fix?) error handling inside of the color() function to throw an error if the user provides a color or grade that does not exist as well as a warning if they're outside of the $alpha channel value range.
  • Lastly, added unit tests for the color() function using True and Mocha! Sadly, I don't think there is a way to test for errors which would have been nice.

.travis.yml Outdated
@@ -5,7 +5,7 @@ deploy:
provider: npm
email: [email protected]
api_key:
secure: XfxcXQvUIlDRiPvKNxsoL3sOEALcDd9uY6FR6TQls3C8nR8+Ocq/WfLYhG53J5Tl0E8EsG/2a9HVTT0+k1RG9bDSthM8ztRm9Y/Fi4ZqN4Wn5BfepOUl24SPqwmG/48w/Wu/cn3wOFEu5h7Y3iiTkcyk5hwJr54THKMH7J313KU=
secure: <needthis>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The original secure key is correct! I need to get a new one for color-bee.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my bad. Changed it back.

@seejamescode
Copy link
Collaborator

@ind1go or @Snugug, can you review the sass generation?

@seejamescode
Copy link
Collaborator

Also @cuyaproductions, does the Readme still reflect this or are there changes in calling the sass?

@diego-codes
Copy link
Contributor Author

diego-codes commented Feb 28, 2017

@seejamescode, I just updated the color examples on the README file to match the current color values and added a small section about running tests.

@panpan-lin
Copy link

panpan-lin commented Mar 29, 2017

Quite a lot of commits in this PR. Not sure about your team but in my team we are told to squash them into 1 or whatever number that actually makes sense, with the ticket number preceding by # as part of the commit message, so that the master branch's history is cleaner and easier to read.

@Snugug
Copy link
Collaborator

Snugug commented Mar 30, 2017

I'd like to help reviewing this PR, but It's way too large with way too many changes to sanely review. Please break this PR up. The issue at hand only mentions not being able to pass just a color name to a Sass mixin, yet I see a whole new testing framework, changes to docs, color palettes, Gulpfile, and sketch files. That's way more code than needed. I'd recommend the following:

Copy link
Collaborator

@Snugug Snugug left a comment

Choose a reason for hiding this comment

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

This PR is too large to review effectively and resolves much more than just the issue it's attached to. Please review my comments from #58 (comment) and resubmit

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting color throws an error if no grade is specified
4 participants