holic / basscss-color-lighten

0 stars 0 forks source link

Thoughts/Feedback #1

Open jxnblk opened 9 years ago

jxnblk commented 9 years ago

Hey, thanks for putting this together – just wanted to share some thoughts and feedback.

  1. Although it's not documented, I've split up the color modules by property in v7. See https://github.com/basscss/background-colors and https://github.com/basscss/border-colors – it would be good to follow a similar organization, but I'm open to splitting the darken and lighten modules apart from the core colors.
  2. I'm not sure I see the value in the .lighten-x styles for foreground colors. It seems like it would cause color contrast issues and set people up for making mistakes (i.e. low-contrast type) in their designs. There is already a .muted style to adjust opacity for text.
  3. Would you eventually want to pass ownership of this module to the Basscss org? There's a few private scripts that I use to manage everything, and I wouldn't want this module to get left behind (Although it does seem fairly feature complete already).
  4. Just putting this out there as an alternative: you could also make a pull request for the background-colors and border-colors modules, including these styles in each one.
holic commented 9 years ago

Just to clarify, I tossed this into its own module mostly to get feedback before opening a pull request. I'm happy to migrate this to the Basscss org if you decide it's best to keep it within its own module.

The .lighten-n styles were actually my motivation for creating this. I'm working with a design that has both white backgrounds and dark backgrounds (e.g. navy). I originally tried using .muted but this also affects things like link styles. This is demonstrated here: https://output.jsbin.com/naqiraqeva

holic commented 9 years ago

In my design, I ended up just creating a .lighten class with color: rgba(255, 255, 255, .66) for more contrast. But this doesn't quite fit your doubling/halfing math throughout Basscss.

jxnblk commented 9 years ago

Good to know. I think the pull request approach would be the best route to go for now.

As far as using the lighten utilities for foreground text, this is similar to the reason why I haven't included darken utilities as foreground colors. With the scale of alpha values being used here, there's almost no way that text would have enough contrast to be readable.

I think the lighten utilities could be added to background-colors and border-colors, but for use as a foreground color, these would require higher alpha values.

As next steps, I'd say:

  1. Make pull requests for the background-color and border-color modules, then I'll merge then in.
  2. (If you want to) Make a separate pull request for foreground colors with higher alpha values, along with screenshot demonstrations and color contrast values against a few different background colors. I want to be sure that using Basscss doesn't easily result in color combinations with contrast lower than 4.5.
holic commented 9 years ago

Understandable. I'll work on that.

jxnblk commented 9 years ago

:sunglasses:

holic commented 9 years ago

Got those pull requests opened.

I dug a bit into colorable and the color module it depends on. It doesn't look like the color testing methods support alpha values. I also tried color's .mix method, but it creates a halfway point between two colors rather than a the result of overlaying a color with alpha on top of another color.

As a workaround, I take the background color and mix it with the foreground color without the alpha value (or alpha=1), then use the foreground's original alpha value for the weight in the mix operation. I then compare the resulting color to the background color for the contrast value.

Here's what I came up with: https://rawgit.com/holic/color-alpha-contrast-test/master/public/index.html

I'm not really sure if my lighten/darken alpha values are the best. I'm open to suggestions!

jxnblk commented 9 years ago

Hey sorry for the delay. I forgot about alpha values not being supported in the color module, but this demo looks great. Thanks for putting it together!

As far as the scale goes, I think it would be simpler to keep the alpha values 1–4 the same as the darken values like you've done in the PRs. For foreground colors, the scale could continue with 5–8. I think this would make it easier to discuss next steps for handling foreground colors.

Here's a table of what I propose trying:

scale alpha value fraction
1 .0625 1/16
2 .125 1/8
3 .25 1/4
4 .5 1/2
5 .625 5/8
6 .75 3/4
7 .875 7/8
8 .9375 15/16
holic commented 9 years ago

Where do you see colors where alpha is below .5 fitting in? Their contrast is already really low (only two above 4.5 at alpha .5) compared to alpha values above that.

jxnblk commented 9 years ago

Sorry if my previous comment wasn't clear. Here's a quick demo of what I was suggesting: http://codepen.io/jxnblk/pen/5a24befa8c6e8dab465a207cdf0ed80d?editors=110

The values for the custom properties might need some tweaking, but this demonstrates how I was thinking about the scale and the utilities available with that scale. It would be nice to keep color utilities parallel with the others, but I don't think foreground colors below .5 alpha make much sense.

holic commented 9 years ago

Ah, got it. Thanks for the demo!

Do you think these additional colors should be under a new module? Or do they fit within the existing ones? Happy to open PRs again or start a new repo that can be migrated to the basscss org.

jxnblk commented 9 years ago

I think they could go within the current modules. If you feel like opening some more PRs, go for it. Otherwise I might take a look at this again in a week or so.