-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Don’t split vars with numbers in them inside arbitrary values #8091
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This normalize function is normalizing the input we get (from the candidate found in your templates) and produce an actual value. So it has to swap _
with
for example. The calc
function in the candidate is defined as calc(1+1)
but that's not valid css, it has to contain spaces around operators like calc(1 + 1)
so that's why the code exists there.
As far as I know this is only necessary for the calc
function to have spaces around operators, but it doesn't hurt to have it on other operators as well for generating cleaner css.
|
||
// Add spaces around some operators not inside calc() that do not follow an operator | ||
// or '('. | ||
return value.replace(/(-?\d*\.?\d(?!\b-.+[,)](?![^+\-/*])\D)(?:%|[a-z]+)?|\))([\/])/g, '$1 $2 ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is strictly necessary, because rgba(0 0 0/0.1)
seems to work as expected, but it generates cleaner css!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because we also apply spaces to aspect-[16/9]. I'd rather remove the spaces if that's fine though.
|
||
// calc(…) get's spaces around operators | ||
['calc(1+2)', 'calc(1 + 2)'], | ||
['calc(100%+1rem)', 'calc(100% + 1rem)'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe add 1 more test with nested calc
just to verify that that works as well calc(1+calc(100%-20px))
for example should result in calc(1 + calc(100% - 20px))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added and it works as expected 🎉
70bb3a9
to
0793759
Compare
A regex we use to normalize spacing around operators in calc was adding spaces around
-
invar(…)
when it was preceded by a number. This fixes that problem. We now also have explicit tests for the normalization behavior instead of just pure indirect tests.Fixes #8079
cc @RobinMalfait I'm not sure I really understand what all is happening here so I would appreciate a quick glance over the code to see if anything jumps out at you