Named color parsing #15128
Named color parsing #15128
Conversation
|
📦 Preview the website for this branch here: https://deploy-preview-15128--ol-site.netlify.app/. |
| if (name === 'transparent') { | ||
| return 'rgba(0,0,0,0)'; | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
is this a good default value? It is a string but no valid rgb(a) string. most likely i would suspect this should not cause an error, because normally if colors are allowed, an empty strings is also ok.
Also this technically differs from the previous implementation because the browsers would actually accept any color name and just parse it randomly. I think this can be neglected as it is an unspecified case - also browsers handle this differently.
There was a problem hiding this comment.
as this is used in fromStringInternal_ and not exported the empty string makes sense.
There was a problem hiding this comment.
The previous behavior in this case was not really predictable. If you el.style.color = 'oops', then getComputedStyle(el).color depends on what other CSS rules might apply. For example, on this page, that returns rgb(31, 35, 40). On about:blank, it returns 'rgb(0, 0, 0)'. I thought that I previously saw a case where the result was '', but cannot reproduce that now.
I think that the new behavior with the exported methods is probably more "correct" now. Calling asArray('oops') now throws new Error('Invalid color') whereas previously I think the behavior would depend on CSS rules that might be on the page.
| expect(() => { | ||
| fromString(c[0]); | ||
| }).to.throwException(expected); | ||
| return; |
There was a problem hiding this comment.
Why does this throw an exception? I would expect an empty string.
There was a problem hiding this comment.
nevermind, this calls fromString not fromNamed
|
Good comments. Thanks for the review. Maybe we will want to revisit the behavior of |
This change updates the
ol/color.jsmodule so it doesn't depend on the DOM.I added a dependency on the
color-namepackage. This is a simple lookup of CSS named colors to RGB values. We could include this lookup directly in theol/color.jsmodule if we don't want the dependency. But it may eventually be useful to depend on https://github.com/colorjs/color-parse or other packages that depend on the same.We didn't previously have tests that checked that
fromString()worked with named colors. I added tests to cover this and moved all color tests to demonstrate that they pass without the DOM.