Conversation
yisibl
force-pushed
the
add-revert-rule
branch
4 times, most recently
from
e6cdc61 to
2f5d56a
Compare
yisibl
force-pushed
the
add-revert-rule
branch
from
2f5d56a to
9545822
Compare
evanw
left a comment
evanw
left a comment
There was a problem hiding this comment.
Thanks for noticing this new CSS keyword and getting this change together. The addition of revert-rule to cssWideAndReservedKeywords looks good to me. But you marked this as a draft so I'm commenting instead of accepting it in case you're still working on it.
| expectPrinted(t, "div { animation: 2s linear 'name 2', 3s infinite 'name 3' }", "div {\n animation: 2s linear name\\ 2, 3s infinite name\\ 3;\n}\n", "") | ||
| } | ||
|
|
||
| func TestCSSWideKeywords(t *testing.T) { |
There was a problem hiding this comment.
What do these tests cover? I could be misunderstanding something but it seems like these tests would behave the same with and without this change, which means the tests in this function aren't covering anything and can be removed.
| expectPrintedMangleMinify(t, "a {font-family: 'unset', serif;}", "a{font-family:\"unset\",serif}", "") | ||
| expectPrintedMangleMinify(t, "a {font-family: 'revert', serif;}", "a{font-family:\"revert\",serif}", "") | ||
| expectPrintedMangleMinify(t, "a {font-family: 'revert-layer', 'Segoe UI', serif;}", "a{font-family:\"revert-layer\",Segoe UI,serif}", "") | ||
| expectPrintedMangleMinify(t, "a {font-family: 'revert-rule', 'Segoe UI', serif;}", "a{font-family:\"revert-rule\",Segoe UI,serif}", "") |
There was a problem hiding this comment.
This test helped me understand the implications of this change. Thank you very much for adding it!
| @@ -1,5 +1,20 @@ | |||
| # Changelog | |||
|
|
|||
| * Add support for the new [`revert-rule`](https://drafts.csswg.org/css-cascade-5/#revert-rule-keyword) CSS-wide keyword ([#4312](https://github.com/evanw/esbuild/pull/4312)) | |||
There was a problem hiding this comment.
I'm not sure this change needs to be called out in the release notes as it's a very subtle change (basically esbuild will now keep quotes around fonts, containers, and animations named revert-rule) that seems unlikely to come up in practice. These release notes make it seem like esbuild has added some kind of support for if() expressions, which is not the case.
There was a problem hiding this comment.
Removing the if() statement also works. I think modifying the example like this might make it easier to understand.
The revert-rule allows CSS custom properties to switch to a predefined value. When --radius-flag is undefined, the revert-rule can reset it to border-radius: 20px. Once a value is defined, it switches to the size you want.
data:text/html;charset=UTF-8,<!DOCTYPE html>
<style>
div {
width: 100px;
height: 100px;
border: 1px solid;
border-radius: 20px;
}
.apply-square {
--radius-flag: 0px;
border-radius: var(--radius-flag, revert-rule);
}
</style>
<div class="apply-square"></div>Therefore, I believe this could become a popular feature in the future, and incorporating it into documentation will help people better understand it.
2 participants
Spec: https://drafts.csswg.org/css-cascade-5/#revert-rule-keyword
CSSWG PR: w3c/csswg-drafts#12992
Blink CL: https://chromium-review.googlesource.com/c/chromium/src/+/6884901