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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add hsla helper using Color model #24

Closed
wants to merge 1 commit into from

Conversation

guzart
Copy link

@guzart guzart commented Nov 26, 2016

My goal was to expose a Polished API while using chroma-js behind the scenes, which is why the Models.Color works as a wrapper for chroma.Color

This might be completely off from the goals of the project, but I hope at least helps to get the conversation stated.

All types of criticism are welcome 馃槃 .

NOTE: Got 100% test coverage with jest --watch but not with jest --coverage, am guessing something to do with babel transpiling?

NOTE: Tried following JSDoc ES2015 class example, but the docs didn't pick the class until I used the @constructor tag, and the Color.hsla method wasn't picked up

@codecov-io
Copy link

codecov-io commented Nov 26, 2016

Current coverage is 100% (diff: 100%)

Merging #24 into master will not change coverage

@@           master   #24   diff @@
===================================
  Files           5     7     +2   
  Lines           9    16     +7   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits            9    16     +7   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update 181d686...fbcbdb0

@bhough
Copy link
Contributor

bhough commented Dec 1, 2016

@guzart Thank you for taking the time to start this an opening the discussion. Even though chroma is fairly light weight, I'd be curious how much of the library will go unused by us. @mxstbr what are your thoughts on an external dependency here?

Also 2 other things @guzart: if you can amend these commits to chores. We are running semantic-release so it will publish new versions automatically with feat commits. Also, you might want to look at #25. That is likely to get merged before we come to a conclusion on this.

@mxstbr
Copy link
Member

mxstbr commented Dec 1, 2016

I'm not a fan of returning a chroma object here, as that's unnecessary API surface area. I don't want to add all of color manipulation, just sass-like lighten(#000) etc.

What do you reckon @bhough?

@bhough
Copy link
Contributor

bhough commented Dec 1, 2016

Agree @mxstbr that was my reasoning for the question about unused code from Chroma. Feels like it has way more than we need, and while convenient to have it available, most of that API would not get used in most cases.

@guzart
Copy link
Author

guzart commented Dec 1, 2016

So color would be a better library to use/wrap?

I like how color splits the getter between .rgb() for a Hash and .rgbArray() for an Array

@guzart guzart changed the title feat(hsla): add hsla helper using Color model chore: add hsla helper using Color model Dec 1, 2016
@bhough
Copy link
Contributor

bhough commented Dec 1, 2016

@guzart possibly, it is definitely closer. Though I would be worried slightly about wrapping it at this stage given that they are doing a pretty thorough rework of the library it seems from reading through the repo.

@guzart
Copy link
Author

guzart commented Dec 1, 2016

Do you think having a Color class as the proxy between conversions is the way to go?

const color = rgb('#1e90ff') // Color()
color.hsla() // { h: 210, s: 1, l: 0.56 }

I also found this libraries:

  • pure-color: handles converting and parsing
  • color-space: color conversions between multiple color spaces, all functions are separate tho there are some interdependencies
  • color-string color parsing

@bhough
Copy link
Contributor

bhough commented Dec 1, 2016

Honestly, I'm not sure that is the way I would go. It would force every color to be stored as a color instance in case you ever wanted to transform it in any way. Also if theming takes off in styled-components for example, I would need to convert all the themed colors to instances of color before I could leverage the color functions in polished. That feels fairly clunky to me for this use case. I imagine users would want to be able to transform any string based color at any point without having to instantiate a new color variable.

I'm certainly open to any counter points on this though.

@mxstbr
Copy link
Member

mxstbr commented Dec 2, 2016

I agree with @bhough, I don't see any value in providing color objects.

@guzart
Copy link
Author

guzart commented Dec 2, 2016

Me neither, I also agree that it would be clunky. I was just copying what Sass does without thinking about the styled-components context 馃憥 .

On a different note, I imagine the hsla helper should take numbers or an object structure and covert it to a CSS rgba value?

css`
  border-color: ${hsla({ h: 210, s: 1, l: 0.56, a: 0.84 })}; // color: rgba(30, 144, 255, 0.84);
  color: ${hsla(210, 1, 0.56, 0.84)}; // color: rgba(30, 144, 255, 0.84);
` 

@mxstbr
Copy link
Member

mxstbr commented Dec 2, 2016

That sounds useful to me! More explicitness yay!

@bhough
Copy link
Contributor

bhough commented Dec 2, 2016

On a different note, I imagine the hsla helper should take numbers or an object structure and covert it to a CSS rgba value?

Yes, this would be great. Anywhere that we can support both styles-as-objects and traditional css properties via strings will be a win. Helps us work with as many approaches as possible.

@mxstbr
Copy link
Member

mxstbr commented Dec 8, 2016

Just checked this out, pure-color looks pretty great! We could use that maybe?

@nikgraf nikgraf mentioned this pull request Dec 12, 2016
@guzart
Copy link
Author

guzart commented Dec 12, 2016

PR does not match the desire API, conversation regarding API in #44

@guzart guzart closed this Dec 12, 2016
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

4 participants