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

[color-swatch] Add support for color difference (Delta E) and contrast #119

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

Conversation

DmitrySharabin
Copy link
Member

@DmitrySharabin DmitrySharabin commented Oct 16, 2024

(in terms of design, related to #96; and is also its evolution)

The main features are:

  • Add support for the vs color to calculate deltas and contrast against
  • The info attribute now allows specific algorithms to be used to calculate DeltaE and contrast. Some usage examples: info="deltaE2000, WCAG21 contrast", info="Contrast: Michelson contrast", info="DeltaE: deltaEITP, DeltaPhi: DeltaPhi contrast" (when specifying the contrast method, the word contrast is mandatory)
  • Add some new props (infoCoords, infoOther, etc) to get the advantage of caching (provided by NudeElement) and make code more DRY
  • The code is more forgiving when wrong coords and/or algorithms for deltas and contrast are specified
  • Not a breaking change

To see it in action: https://deploy-preview-119--color-elements.netlify.app/src/color-swatch/#the-vs-attribute

Copy link

netlify bot commented Oct 16, 2024

Deploy Preview for color-elements ready!

Name Link
🔨 Latest commit a839910
🔍 Latest deploy log https://app.netlify.com/sites/color-elements/deploys/67615741b4baae0008ac514a
😎 Deploy Preview https://deploy-preview-119--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.

@LeaVerou
Copy link
Member

Why do we have two different syntaxes to parameterize what algorithm is used? For ΔE we use the function name (deltaE2000) but for contrast we use space separated identifiers. We should pick one syntax and go with it.

Also, thinking of how extends to the color scale case, do we want to be duplicating colors all over the place? Or do we want some way to "link" to another <color-swatch> somewhere else? But then how does this work in the <color-scale> case where there is no (light DOM) <color-swatch> to link to?

@DmitrySharabin
Copy link
Member Author

Why do we have two different syntaxes to parameterize what algorithm is used? For ΔE we use the function name (deltaE2000) but for contrast we use space separated identifiers. We should pick one syntax and go with it.

I just thought of another option. What if we adopt the way we used to specify color space and coordinate in one go and do the same thing with ΔE and contrast? For example, info="deltaE.2000, contrast.WCAG21". It's aligned with the rest of the info value (if specified) and makes it look more “balanced,” if I can say so.

Also, thinking of how extends to the color scale case, do we want to be duplicating colors all over the place? Or do we want some way to "link" to another <color-swatch> somewhere else? But then how does this work in the <color-scale> case where there is no (light DOM) <color-swatch> to link to?

Interesting. I need to think about it.

@LeaVerou
Copy link
Member

Why do we have two different syntaxes to parameterize what algorithm is used? For ΔE we use the function name (deltaE2000) but for contrast we use space separated identifiers. We should pick one syntax and go with it.

I just thought of another option. What if we adopt the way we used to specify color space and coordinate in one go and do the same thing with ΔE and contrast? For example, info="deltaE.2000, contrast.WCAG21". It's aligned with the rest of the info value (if specified) and makes it look more “balanced,” if I can say so.

I like it!

@DmitrySharabin
Copy link
Member Author

Updated usage examples: info="deltaE.2000, contrast.WCAG21", info="Contrast: contrast.Michelson", info="DeltaE: deltaE.ITP, DeltaPhi: contrast.DeltaPhi"

@DmitrySharabin DmitrySharabin force-pushed the deltas-and-contrast branch 2 times, most recently from 0f4034e to fbce8cf Compare October 24, 2024 12:54
@@ -131,7 +131,7 @@ future_swatch_container.append(swatch);
### The `info` attribute

You can use the `info` attribute to show information about the color.
Currently, the only type of information supported is color coords (in any color space), but more will be added in the future.
Currently, the types of information supported are color coords (in any color space), the difference (delta) and contrast between the current color and another one (specified via [the `vs` attribute](./#the-vs-attribute)).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Currently, the types of information supported are color coords (in any color space), the difference (delta) and contrast between the current color and another one (specified via [the `vs` attribute](./#the-vs-attribute)).
Currently, the types of information supported are color coords (in any color space), the difference (deltaE) and contrast between the current color and another one (specified via [the `vs` attribute](./#the-vs-attribute)).

delta and deltaE are different things.

Copy link
Member

Choose a reason for hiding this comment

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

That said, we may want to think about how to support actual deltas (e.g. hue difference)

Copy link
Member Author

Choose a reason for hiding this comment

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

We already do it in the PR (see the colorDeltas prop). For hues, we constrain angles using the shorter arc.

@DmitrySharabin
Copy link
Member Author

There's so much happening around <color-swatch> now:

I wonder what PR we should get done and merge first. There will definitely be merge conflicts. 😭

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.

Could you add comments above the new props about what kind of info they include? I haven't looked at this in a while and I'm a bit lost

@DmitrySharabin
Copy link
Member Author

Could you add comments above the new props about what kind of info they include? I haven't looked at this in a while and I'm a bit lost

Done. Please let me know if I need to add some more.

DmitrySharabin and others added 2 commits December 17, 2024 11:46
@DmitrySharabin DmitrySharabin force-pushed the deltas-and-contrast branch 2 times, most recently from 0f303aa to a839910 Compare December 17, 2024 10:49
@@ -338,6 +358,8 @@ These properties are read-only.
| `--transparency-cell-size` | `<length>` | The size of the cells of the transparency gradient. |
| `--transparcency-background` | `<color>` | The background color of the transparency gradient. |
| `--transparency-darkness` | `<percentage>` | The opacity of the black color used for dark parts of the transparency gradient. |
| `--positive-delta-color` | `<color>` | The color used for the positive color difference in color coords. |
| `--negative-delta-color` | `<color>` | The color used for the negative color difference in color coords. |
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what these mean.

@@ -7,6 +7,8 @@
0 0 / calc(2 * var(--_transparency-cell-size)) calc(2 * var(--_transparency-cell-size))
content-box border-box var(--_transparency-background)
);
--_positive-delta-color: var(--positive-delta-color, hsl(120, 80%, 25%));
--_negative-delta-color: var(--negative-delta-color, hsl(0, 85%, 40%));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you don't need commas.

},
},
/**
* Specified deltaE and contrast
Copy link
Member

Choose a reason for hiding this comment

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

should this be "or contrast"?

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