fix(runtime-dom): undefined cssvars should not be bound to the element #7475
Conversation
|
I tried adding a unit test, but there is a difference in how the |
| @@ -80,7 +80,9 @@ function setVarsOnNode(el: Node, vars: Record<string, string>) { | |||
| if (el.nodeType === 1) { | |||
| const style = (el as HTMLElement).style | |||
| for (const key in vars) { | |||
| style.setProperty(`--${key}`, vars[key]) | |||
| if(vars[key]){ | |||
There was a problem hiding this comment.
Just want to note this condition will prevent setting empty strings to the property.
I think there is at least one use case where this might be preferred, as an empty string would be a valid value for a content property.
There was a problem hiding this comment.
I agree with your opinion 😊
There was a problem hiding this comment.
Just want to make sure this comment is not holding up this PR. It's good since 91a2dc6, but there is no option for me to resolve this comment.
❌ Deploy Preview for vuejs-coverage failed.
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
baiwusanyu-c
force-pushed
the
bwsy/fix/CSSVarsUndefined
branch
from
f231b9c
to
88f0e09
Compare
Size ReportBundles
Usages
|
CodSpeed Performance ReportMerging #7475 will not alter performanceComparing Summary
|
|
@baiwusanyu-c, is this PR ready to go through? I can try to fix the failing test, if that is all that is left. |
haoqunjiang
added
🧹 p1-chore
|
There's also a potential problem with components that render recursively. If the inner component doesn't set the variable then it will be inherited from the outer component: Setting a variable to |
I think the answer is yes, as I would not expect Is there anything else holding up this PR? |
close: #7474