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

Add $contrast as third parameter of contrast-color() function. #4

Closed
wants to merge 1 commit into from

Conversation

imkremen
Copy link

@imkremen imkremen commented Mar 26, 2018

Add $contrast as third parameter of contrast-color() function. By default contrast set to 21, to not brake legacy behavior of function. #3

Add $contrast as third parameter of contrast-color() function. By default contrast set to 21, to not brake legacy behavior of function.
@imkremen imkremen changed the title Add $contrast as third parameter of contrast-color() function. #3 Add $contrast as third parameter of contrast-color() function. Mar 26, 2018
@imkremen imkremen changed the title Add $contrast as third parameter of contrast-color() function. Add $contrast as third parameter of contrast-color() function. #3 Mar 26, 2018
@xi
Copy link
Owner

xi commented Mar 26, 2018

This looks like a good idea. Still, please give me some days to think about it. There are many ways in which the API could be extended and I want to be sure that this is the best way to go.

Apart of that, there are some small changes I would like to suggest:

  • contrast($color1, $base) should be cached
  • The documentation should be extended
  • There should be some test cases for the new behavior

But maybe it is best to wait with these changes until I have made up my mind.

Thanks anyway for this awesome pull request!

@imkremen
Copy link
Author

@xi Thanks you for awesome library.
Today I start using it in the big project, so I'll try to help you extend some features and documentation in future.

@imkremen imkremen changed the title Add $contrast as third parameter of contrast-color() function. #3 Add $contrast as third parameter of contrast-color() function. Mar 27, 2018
@xi
Copy link
Owner

xi commented Mar 29, 2018

I thought about this for a bit. Sorry if this is a bit longer.

My primary concern is that you could expect contrast-color to do any number of things. There is no "one true implementation". Maybe it is time to split things into two categories: A high level function that returns a color with decent contrast and low level functions that allow fine grained control.

I think you can categorize the functions in the contrast module along 3 dimensions: How many input colors do they take? Which one do they select? What do they do if none of the colors provides sufficient contrast? The existing functions can then be described like this:

  • contrast-color
    • 2 input colors
    • the better one is selected
    • as there is always a better one, there is no error case
  • contrast-color with you modification
    • 2 input colors
    • the first one with sufficient contrast is selected
    • if none has suficient contrast, the better on is selected
  • contrast-stretch
    • 1 input color
    • as there is only 1 input, there is no selection
    • if it does not provide sufficient contrast it is stretched as far as possible/needed
  • contrast-check
    • 1 input color
    • as there is only 1 input, there is no selection
    • if it does not provide sufficient contrast there is a warning

I thought of 2 more functions that might be a nice addition (see 272657f for an implementation):

  • contrast-first
    • n input colors
    • the first one with sufficient contrast is selected
    • return null
  • contrast-best
    • n input colors
    • the best one is selected
    • as there is always a best one, there is no error case

We could then think of contrast-stretch, contrast-check, contrast-first, and contrast-best as the low-level functions. There would still be a default implementation of a high-level contrast-color(), but users could easily add their own. Here is just on example of contrast-color that you could be build on top of this system:

@function contrast-color($base, $color1, $color2, $threshold) {
  $c: contrast-first($base, ($color1, $color2), $threshold);
  @if $c == null {
    $c: contrast-best($base, ($color1, $color2));
    $c: contrast-stretch($base, $c, $threshold);
    $_: contrast-check($base, $c, $threshold);
  }
  @return $c;
}

I now have two questions:

  • Is this overkill? Would new users know which functions they need to use?
  • @imkremen would this help you with your usecase?

@imkremen
Copy link
Author

Here how is see it for now (I need some time to think about it):

/// Pick contrast option for a given base color
///
/// @param {color} $base the base color to compare to
/// @param {list} $colors [($planifolia-contrast-dark-default, $planifolia-contrast-light-default)] list of colors
/// @param {number} $ratio [21] satisfactory contrast ratio between 1 and 21
/// @return {color | false} color from list `$colors` or false
@function contrast-pick(
  $base,
  $colors: (
    $planifolia-contrast-dark-default,
    $planifolia-contrast-light-default,
  ),
  $ratio: 21
) {
  $ratio: _pf-threshold($ratio);
  $length: length($colors);
  $best: false;

  @if $length < 1 {
    @error "#{$colors} need minimum one item"
  }
  
  @for $i from 1 through $length {
    $color: nth($colors, $i);

    @if contrast($color, $base) >= $ratio {
      @return $color;
    }
    
    @if $i < $length {
      $nextColor: nth($colors, $i+1);

      @if contrast($color, $base) >= contrast($nextColor, $base) {
        $best: $color;
      } @else {
        $best: $nextColor;
      }
    }
  }
  @return $best;
}

.elem {
  $bg: black;
  $fg: contrast-pick($bg, (blue, #008A00, red), 4.5);
  background: $bg;
  color: $fg;
  padding: 8px;
  &:before {
    content: "#{$fg}";
  }
}

@xi
Copy link
Owner

xi commented Apr 8, 2018

I have thought about this again and decided that I will not merge it.

The important logic of this library is in the contrast() function. contrast-color() is there for convenience. If you need more control, you can easily write your own functions as demonstrated in 272657f. Adding these low-level functions to the library would just clutter the API.

Thanks for the idea though. I really appreciate it!

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