FIrst stab at #212 #213
FIrst stab at #212 #213
Conversation
✅ Deploy Preview for color-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
DmitrySharabin
force-pushed
the
issue-212
branch
from
be1bc59 to
281bec1
Compare
DmitrySharabin
force-pushed
the
issue-212
branch
2 times, most recently
from
c62524b to
d671338
Compare
DmitrySharabin
force-pushed
the
issue-212
branch
from
d671338 to
92f46d0
Compare
src/common/color-element.js
Outdated
| // One of the supported color values; resolve and cache it | ||
| element.style.backgroundColor = value; | ||
| let color = getComputedStyle(element).backgroundColor; | ||
| color = Self.Color.get(color); |
There was a problem hiding this comment.
This can still error (e.g. if the color includes currentColor I believe it won't resolve fully), I would just call resolveColor() with a check to make sure it has actually changed (otherwise you'll be in an infinite loop).
We should also handle the case where value is non-empty and color is null better across components.
src/common/color-element.js
Outdated
| element.style.backgroundColor = value; | ||
| let color = getComputedStyle(element).backgroundColor; | ||
| color = Self.Color.get(color); | ||
| Self.#resolvedColors.set(value, color); |
There was a problem hiding this comment.
One issue with this is that now multiple components can share the same Color object if they use the same string, and I'm pretty sure we modify it. So perhaps we should cache the resolved string and create a new instance every time.
src/common/color-element.js
Outdated
| let color = getComputedStyle(element).backgroundColor; | ||
| element.style.backgroundColor = ""; | ||
|
|
||
| let resolvedColor = Self.resolveColor(color, element); |
There was a problem hiding this comment.
You need to check if getComputedStyle() actually produced a different color, otherwise this can end up an infinite loop. Or, better, just don't pass an element, and write the code accordingly so that when no element is passed it just doesn't use gCS().
There was a problem hiding this comment.
Nice idea! Thanks. I hope I got it right now. 🫣
Co-authored-by: Lea Verou <lea@verou.me>
| } | ||
|
|
||
| // One of the supported color values; resolve and cache it | ||
| element.style.backgroundColor = value; |
There was a problem hiding this comment.
We should store the element's previous backgroundColor so we can restore it, rather than overwriting.
Also, we should avoid the DOM I/O if the element already has that background (since any DOM operation is slow), so perhaps we can pass in an option for that.
There was a problem hiding this comment.
We should store the element's previous
backgroundColorso we can restore it, rather than overwriting.
I fixed it. Thanks!
Also, we should avoid the DOM I/O if the element already has that background (since any DOM operation is slow), so perhaps we can pass in an option for that.
Could you elaborate, please? How do you see it?
There was a problem hiding this comment.
Could you elaborate, please? How do you see it?
Like, a setting (name TBD) that will tell the function "the element I'm giving you already has the right background, don't bother setting it"
2 participants
[color-swatch] Handle complex color values gracefully
This is part 1 of 2 in a stack made with GitButler: