Skip to content

[color-scale] Experiments with handling complex CSS colors #214

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.

Sign up for GitHub

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

Open
DmitrySharabin wants to merge 1 commit into
base: issue-212
Choose a base branch
from
Open

[color-scale] Experiments with handling complex CSS colors #214

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/color-scale/color-scale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,13 @@ const Self = class ColorScale extends ColorElement {
is: Object,
// Support overriding the Color object
get values () {
return ColorScale.Color;
class Color extends ColorScale.Color {
constructor (value) {
let color = ColorScale.resolveColor(value, dummy);
super(color);
}
}
return Color;
},
defaultKey: (v, i) => v,
},
Expand Down Expand Up @@ -165,3 +171,9 @@ const Self = class ColorScale extends ColorElement {
Self.define();

export default Self;

let dummy;
if (document) {
dummy = document.createElement("div");
document.body.appendChild(dummy);
Copy link
Member

@LeaVerou LeaVerou Aug 5, 2025

Choose a reason for hiding this comment

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

We absolutely don't want our component modifying the light DOM to add elements, that's very much seen as an antipattern. But also, resolving based on <body> values is suboptimal, we want to resolve on the element itself.
If we went that route, we could have a shadow DOM dummy element to resolve with, but I'm not sure we need it. color-scale uses color-swatch, and all the resolving should happen there. Why does color-scale need to resolve anything?

Copy link
Member Author

@DmitrySharabin DmitrySharabin Aug 5, 2025

Choose a reason for hiding this comment

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

Okay, I'll give it another try

Copy link
Member Author

@DmitrySharabin DmitrySharabin Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why does color-scale need to resolve anything?

<color-scale>s have the colors prop that should be initiated and parsed before any <color-swatch> can be created and rendered. If we don't resolve colors, Color.js will throw an error for any unsupported color, and the scale's color property will remain undefined. As a result, no color swatches will be created.

I might miss something, but I don't see a way to resolve colors on <color-swatch>s instead of on <color-scale>s. 🤷‍♂️

As an example:

image image

Copy link
Member Author

@DmitrySharabin DmitrySharabin Aug 5, 2025

Choose a reason for hiding this comment

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

Oh, and I tried to use the element itself to resolve colors, but at the time the colors prop is initialized, this corresponds to the prop, not the element. So, if we decide to use a dummy element in the element's shadow DOM, I'm unsure how to access it when we need it.

Copy link
Member

@LeaVerou LeaVerou Aug 6, 2025

Choose a reason for hiding this comment

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

Hmm. Let me think about it. I wonder if the right solution involves changes to Color.js and Nude Element. I have some ideas.

}