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

Fix lightness manipulators lighten and darken. #92

Closed
wants to merge 1 commit into from

Conversation

ivansky
Copy link

@ivansky ivansky commented Jul 10, 2016

Lightness functions should work in percent. #91

Lightness functions should work in percent.
@Qix-
Copy link
Owner

Qix- commented Jul 11, 2016

// @mattbasta @sindresorhus @MoOx can you confirm this is what is expected? This is a breaking change :P

@MoOx
Copy link
Collaborator

MoOx commented Jul 11, 2016

Per examples here https://github.com/Qix-/color#manipulation many functions seems to be modifiers, so act on the current existing value (so current - current * value) make sense.
If you want to change this behavior for one method, you should do it for all method.
Otherwise, like CSS color functions, you can accept +10% (modify current state, like it's currently) or 10% (brutal change as this PR).

Note: maybe I am wrong, I am not especially a pro about color manipulation :)

@mattbasta
Copy link

Both are correct, because each is a different blending function. Each has a valid use case. The question is which is most expected. To me, the existing behavior makes the most sense.

@ivansky
Copy link
Author

ivansky commented Jul 11, 2016

Many developers will be confused
These blending functions work in other libraries as I proposed.
So, I suggest leave existing functions as is with other names and apply my changes in new major version.
I don't know how to name the current functions. They should not be named as lighten or darken.

@MoOx
Copy link
Collaborator

MoOx commented Jul 11, 2016

By other libraries you mean Sass and LESS?
Because I didn't see a lot of complain about that overthere :)

@ivansky
Copy link
Author

ivansky commented Jul 11, 2016

For example

Current library has 48 issues. So I guess, issues will be appear again until we resolve it.

@MoOx
Copy link
Collaborator

MoOx commented Jul 11, 2016

Just one thing imo: you cannot just remove/replace a function. This module has been downloaded so many times...
So I would go for breaking change but keep the old method (for the naming, I don't care :)).
Maybe we can add a parameter to switch between absolute or relative?
In any case, should be adjusted to all similar methods.

@ivansky
Copy link
Author

ivansky commented Jul 11, 2016

I agree with you. We need to support users. I think, we have 3 options:

  1. Leave the old names and add a boolean parameter as third argument of function.
  2. Add this breaking change to new major version.
  3. Add new functions and do not support the same naming

I like second option as you can see :)

@MoOx
Copy link
Collaborator

MoOx commented Jul 11, 2016

Adding a breaking change is fine, but you should still offer the relative implementation (eg: relativeLighten or something).

@ivansky
Copy link
Author

ivansky commented Jul 11, 2016

So, I think name relative implementation as lighter and darker.
Or brighten and what? I don't know a good antonym.
I am not sure, because names should look conspicuous.
Is it fine or not? Please, could you help me with naming.
Unfortunately, I haven't enough knowledge in english.

@Qix-
Copy link
Owner

Qix- commented Nov 16, 2016

This won't make it into the v1.x makeover, but I'd like to look into this more. I agree that those functions are super confusing.

I'll dig a bit deeper, see the differences with them applied to a few images, and maybe suggest a few names and whatnot.

@lvl99
Copy link

lvl99 commented Mar 3, 2017

I'd love to set an absolute value for the saturationl of the color. Maybe a generic term like brightness could be the handle for the absolute, and lighten and darken can remain relative.

@sammi
Copy link

sammi commented May 21, 2018

I think we keep the existing name: darken, lighten, saturation, desaturation, do abolute percentage changes makes sense. We may need regard the relative changes as a bug, not a feature. So it would not be a concern for break back compatible ablity.

@Qix-
Copy link
Owner

Qix- commented Oct 22, 2018

I'm closing this, upon further review this is not the way this should work (it's a breaking change). Please see #53, as that should be the definitive place for discussion.

@Qix- Qix- closed this Oct 22, 2018
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.

6 participants