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

First stab on adding support for deltas and contrast from other color #96

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

DmitrySharabin
Copy link
Member

@DmitrySharabin DmitrySharabin commented Jun 11, 2024

I chose vs as the attribute name for the other color. We can easily change it if needed.

For now, it looks like this:

image

Copy link

netlify bot commented Jun 11, 2024

Deploy Preview for color-elements ready!

Name Link
🔨 Latest commit 09f4136
🔍 Latest deploy log https://app.netlify.com/sites/color-elements/deploys/6670759649e2a00008f29b72
😎 Deploy Preview https://deploy-preview-96--color-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@DmitrySharabin DmitrySharabin requested a review from LeaVerou June 11, 2024 16:56
@LeaVerou
Copy link
Member

Could you add some docs so I can review the API too?

@DmitrySharabin
Copy link
Member Author

Could you add some docs so I can review the API too?

Sure

@DmitrySharabin
Copy link
Member Author

Could you add some docs so I can review the API too?

Sure

Done!

Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

  • We don’t want to hardcode a specific deltaE algorithm, we should allow customizing it (it's unclear which one is being hardcoded here). I think this should be fine as part of coords. coords="L: oklch.l, C: oklch.c, H: oklch.h, ΔΕ: deltaE2000". It should be easy to disambiguate the two. Perhaps we should even rename coords to data to not be confusing?
  • Don't use deltas() yet, since this is not part of any released Color.js version. I just mentioned it so you could look at the implementation. Though after seeing the hue charts, not sure its approach would be the right one for hue
  • We don't want to have to style two parts for (visually) the same thing.
  • Not sure we want a separate boolean attribute. We can use vs without a value to opt-in, and with a value to specify a specific color.

Use `vs` without a value to opt-in deltas (from the previous color) and with a value to specify a specific color.
* Rename: `coords` → `data`
* Allow specifying `ΔΕ` algorithm (uses parameterized syntax under the hood)
* Combine coords and `ΔΕ` in one part—data
* Fix styles accordingly
* Update docs
We can't use `deltas()` yet. It's not in any released version of Color.js.
@DmitrySharabin
Copy link
Member Author

Thank you for your feedback. I addressed all the issues you mentioned. Here is the summary of the changes:

  • Rename: coordsdata
  • Allow specifying ΔΕ algorithm (uses parameterized syntax under the hood)
  • Combine coords and ΔΕ in one part—called “data.” Fix styles accordingly
  • Calculate deltas manually (since we can't use deltas() yet)
  • Update docs
  • [color-scale] Use vs without a value to opt-in deltas (from the previous color) and with a value to specify a specific color.

@DmitrySharabin DmitrySharabin requested a review from LeaVerou June 12, 2024 21:28
@DmitrySharabin DmitrySharabin changed the title First stab on adding support for deltas from other color First stab on adding support for deltas and contrast from other color Jun 17, 2024
// If there is a vs color, use it as the reference for the deltas
swatch.vs = this.vs;
}
else if (this.#hasVs && i > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why would you check this and not this.vs !== undefined or something?

@@ -153,6 +166,9 @@ const Self = class ColorScale extends NudeElement {
additionalDependencies: ["info"],
},
info: {},
vs: {
type: Color,
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see why you're doing the #hasVs thing. That’s not the right way; it means that if someone sets it via the property you get a different behavior. You want two properties: one for the raw value, and one for the parsed value, which is either true (previous color), false (no vs), or a Color instance.

And yes, this is another case of rawProp.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm doing this precisely because I can't see another way to distinguish different cases: color, no color, no vs.

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be to use a custom parse function. Up to you.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it may be useful to provide a way to do vs with the previous or next color. In that case we could have vs="previous" and vs="next" which may be more explanatory than a bare vs with no value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option would be to use a custom parse function.

Yeah, I was going to try this later. Glad it makes sense. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if it may be useful to provide a way to do vs with the previous or next color. In that case we could have vs="previous" and vs="next" which may be more explanatory than a bare vs with no value.

I like it

Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

In addition to the other comments, the info is useful to get programmatically as well. Don't just calculate them inline, set a getter prop with them!

(same applies to info actually, so you probably want a data structure that supports both. Let's discuss the data structure before you implement)

@LeaVerou
Copy link
Member

Some ideas:

Option 1: Two separate data structures

colorInfo: {"oklch.l": 0.5, deltaE2000: undefined, contrastAPCA: undefined},
vsInfo: {"oklch.l": 0.2, deltaE2000: 10, contrastAPCA: 25},

Option 2: One data structure, one entry per key

Option 2.1: Object when 2 values, value otherwise

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2}, 
	deltaE2000: 10, 
	contrastAPCA: 25
},

Option 2.2: Object always

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2}, 
	deltaE2000: {value: 10, delta: 10}, // or {value: undefined, delta: 10}?
	contrastAPCA: {value: 25, delta: 25}, // or {value: undefined, delta: 25}?
},

What do you think are the pros and cons of each approach? Do you see any other options?

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Jun 17, 2024

Option 1: Two separate data structures

colorInfo: {"oklch.l": 0.5, deltaE2000: undefined, contrastAPCA: undefined},
vsInfo: {"oklch.l": 0.2, deltaE2000: 10, contrastAPCA: 25},

Pros

  • All the needed info about the color (the current one and vs) in one place
  • Both colorInfo and vsColor have the same (predictable) structure
  • All colorInfo and vsColor properties are scalars; no need for extra checks

Cons

  • Both colorInfo and vsInfo should stay in sync: when one changes, the other should be recalculated (it may lead to a circular dependency)
  • When we need deltas, we have to recalculate them based on values in two properties instead of simply reading them from one (from colorInfo)
  • When we need info concerning the current color (e.g., deltaE and contrast), we should get it from another property—vsInfo

Option 2.1: Object when 2 values, value otherwise

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2}, 
	deltaE2000: 10, 
	contrastAPCA: 25
},

Pros

  • All the needed info is calculated once, stored in one place, and can be cached
  • No circular dependencies: colorInfo is updated on color and vs change

Cons

  • On every property read, we should check whether its value is an object or a scalar. OTOH, we can easily see which properties are coords (objects) and which are not (scalars)

Option 2.2: Object always

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2}, 
	deltaE2000: {value: 10, delta: 10}, // or {value: undefined, delta: 10}?
	contrastAPCA: {value: 25, delta: 25}, // or {value: undefined, delta: 25}?
},

Pros

  • All the needed info is calculated once, stored in one place, and can be cached
  • No circular dependencies: colorInfo is updated on color and vs change
  • On reading a property value, there is no need to check whether it's an object or a scalar: all properties have the same structure—{value, delta}

Cons

  • In deltaE2000: {value: 10, delta: 10} and contrastAPCA: {value: 25, delta: 25}, delta doesn't make sense (?); we need value only. OTOH, we can use this to distinguish coords from other data

I would also add the isAngle (angle?) property to display deltas correctly (as a percentage or a number without any units). It can be calculated once and read when needed.

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2, isAngle: false}, 
},

Another option would be to provide the unit itself.

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2, unit: "%"}, 
},

However, in that case, we should probably either adjust delta depending on the unit or calculate it with this unit in mind. Or, alternatively, we could have rawDelta (calculated) and delta (adjusted):

colorInfo: {
	"oklch.l": {value: 0.5, rawDelta: 0.2, delta: 20, unit: "%"}, 
},

If we go with option 2.2, should we also add these properties (rawDelta, unit; which should be undefined to my mind) to deltaE2000 and contrastAPCA? Luckily, if an object doesn't have a property, it's considered undefined.

- Remove `#hasVs`
- Use custom `parse()`
- Add support for `previous` and `next` keywords
@LeaVerou
Copy link
Member

This is mostly spot on. Some comments on the analysis:

Option 1: Two separate data structures

colorInfo: {"oklch.l": 0.5, deltaE2000: undefined, contrastAPCA: undefined},
vsInfo: {"oklch.l": 0.2, deltaE2000: 10, contrastAPCA: 25},

Pros

  • All the needed info about the color (the current one and vs) in one place
  • Both colorInfo and vsColor have the same (predictable) structure
  • All colorInfo and vsColor properties are scalars; no need for extra checks

Cons

  • Both colorInfo and vsInfo should stay in sync: when one changes, the other should be recalculated (it may lead to a circular dependency)

How would it lead to a circular dependency? colorInfo depends on color and info vsInfo depends on colorInfo (or color), vs, and info. I don’t see a cycle?

image

If anything, another pro of this approach is that it separates the dependencies: if only the vs color changes, we don’t need to also update colorInfo.

  • When we need deltas, we have to recalculate them based on values in two properties instead of simply reading them from one (from colorInfo)
  • When we need info concerning the current color (e.g., deltaE and contrast), we should get it from another property—vsInfo

What do you mean? deltaE and contrast don't make sense for a single color, they are inherently operators that take two colors.

Option 2.1: Object when 2 values, value otherwise

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2}, 
	deltaE2000: 10, 
	contrastAPCA: 25
},

Pros

  • All the needed info is calculated once, stored in one place, and can be cached
  • No circular dependencies: colorInfo is updated on color and vs change

Cons

  • On every property read, we should check whether its value is an object or a scalar. OTOH, we can easily see which properties are coords (objects) and which are not (scalars)

I think that's a pretty big downside fwiw. It makes using it quite annoying:

element.colorInfo["oklch.l"]?.value ?? element.colorInfo["oklch.l"]

Yikes.

Option 2.2: Object always

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2}, 
	deltaE2000: {value: 10, delta: 10}, // or {value: undefined, delta: 10}?
	contrastAPCA: {value: 25, delta: 25}, // or {value: undefined, delta: 25}?
},

Pros

  • All the needed info is calculated once, stored in one place, and can be cached
  • No circular dependencies: colorInfo is updated on color and vs change
  • On reading a property value, there is no need to check whether it's an object or a scalar: all properties have the same structure—{value, delta}

Cons

  • In deltaE2000: {value: 10, delta: 10} and contrastAPCA: {value: 25, delta: 25}, delta doesn't make sense (?); we need value only.

Think of it from the perspective of code that handles this info generically, without knowing which ones apply to one color and which ones to two. If we only have value there’s no way to tell (without duplicating the logic about which one is what in calling code). But that could be addressed with a flag.

OTOH, we can use this to distinguish coords from other data

Yup!

I would also add the isAngle (angle?) property to display deltas correctly (as a percentage or a number without any units). It can be calculated once and read when needed.

I’ll need to think about this more.

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2, isAngle: false}, 
},

Another option would be to provide the unit itself.

colorInfo: {
	"oklch.l": {value: 0.5, delta: 0.2, unit: "%"}, 
},

However, in that case, we should probably either adjust delta depending on the unit or calculate it with this unit in mind. Or, alternatively, we could have rawDelta (calculated) and delta (adjusted):

colorInfo: {
	"oklch.l": {value: 0.5, rawDelta: 0.2, delta: 20, unit: "%"}, 
},

If we go with option 2.2, should we also add these properties (rawDelta, unit; which should be undefined to my mind) to deltaE2000 and contrastAPCA? Luckily, if an object doesn't have a property, it's considered undefined.

Not sure about this either, since we have no way of selecting a unit currently.


Another con of 2.2 is that long chains of properties are annoying and error prone. Having to do element.colorInfo['oklch.l'].value to get the value is tedious AF.


So weighing these tradeoffs, which option would you go with?

Personally I’m leaning toward Option 1, but need to hear more about the circular dependency in case I’m missing something. We could even have a vsColorInfo property to cache the other color’s info to partition the dependencies even more.

@DmitrySharabin
Copy link
Member Author

How would it lead to a circular dependency? colorInfo depends on color and info vsInfo depends on colorInfo (or color), vs, and info. I don’t see a cycle?

You are right; there is no cycle. My brain simply refuses to work properly today. 🤦‍♂️ Thank you for checking it.

What do you mean? deltaE and contrast don't make sense for a single color, they are inherently operators that take two colors.

I looked at your example code and noticed that the deltaE and contrast info is stored in vsInfo: {"oklch.l": 0.2, deltaE2000: 10, contrastAPCA: 25}, in colorInfo those properties are undefined. But I thought that we calculate deltaE and contrast for the current color (using the vs color as the second one), so, all the relevant info should be stored in colorInfo. While I was writing this, it dawned on me that it makes more sense to store this data in vsInfo, as you suggested: current value for the chosen color coord, distance from the current color, and contrast with the current color—all these pieces of data are relevant to the vs color.

I think that's a pretty big downside fwiw. It makes using it quite annoying:

element.colorInfo["oklch.l"]?.value ?? element.colorInfo["oklch.l"]

Yikes.

Agreed!

So weighing these tradeoffs, which option would you go with?

After re-reading this one more time, I'm also leaning toward Option 1. It seems like it will also make the code more readable than we might get with other options.

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.

2 participants