Skip to content

Conversation

@cscheid
Copy link
Collaborator

@cscheid cscheid commented Apr 14, 2025

Closes #11665.

(@gordonwoodhull, I'm asking for a review from you because I think you've written CSS color detectors in your typst work. I'd love to know if we're missing anything with isCssColorName specifically. Thanks!)

@cscheid cscheid requested review from cderv and gordonwoodhull April 14, 2025 16:04
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

I added a simple test for now just about the presence of the warning so we don't lose it in the future.

Testing for the revealjs error seems odd as it happens only in a specific case with background-color

I'll also improve printMessage verify function in another PR so we can test multiple regexes.
I'll modify this test there so that we test also for no warn on some known color.

if (name.startsWith("#")) return true;
if (name.startsWith("rgb")) return true;
if (name.startsWith("hsl")) return true;
if (name.startsWith("hwb")) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot more color spaces than this, but this should be fine.

"white",
"whitesmoke",
"yellow",
"yellowgreen",
Copy link
Contributor

@gordonwoodhull gordonwoodhull Apr 14, 2025

Choose a reason for hiding this comment

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

Yes, looks the same as the named colors in typst_css.lua.

transparent is also important, once a keyword, now a color

I see both of us missed rebeccapurple, the other color defined after the original spec.

@cscheid cscheid merged commit d565062 into main Apr 14, 2025
49 checks passed
@cscheid cscheid deleted the bugfix/11665 branch April 14, 2025 19:20
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.

background doesn't work as a color in revealjs unless it's explicitly set

4 participants