fix(useCssVar): new Behavior #4500
Conversation
|
Should this work? If so, then this is a new bug. <script setup lang="ts">
import { useCssVar } from '@vueuse/core'
import { ref } from 'vue'
const el = ref(null)
const color = useCssVar('--color', window.document.documentElement, { initialValue: '#df8543' })
function switchColor() {
if (color.value === '#df8543')
color.value = '#7fa998'
else
color.value = '#df8543'
}
</script>
<template>
<div ref="el" style="color: var(--color)">
Sample text, {{ color }}
</div>
<button @click="switchColor">
Change Color
</button>
</template> |
Do you mean the color switch? |
|
但是依旧没有解决 However, the issue of |
When I load the page, the color state changes to If this isn't the expected behavior, we can fix it in this PR. 2025-01-17.10.00.04.mp4 |
對，這個 PR 主要是修復關於在 Yes, this PR mainly fixes the issue where the |
I dont think this is expected. maybe i am wrong :) |
packages/core/useCssVar/index.ts
Outdated
| { immediate: true }, | ||
| { flush: 'post' }, |
There was a problem hiding this comment.
dont we need both (post and immediate)? And if not why? 😄
There was a problem hiding this comment.
I think we don't need immediate because lines 54-55 are used to clean up the prop from the previous element.
vueuse/packages/core/useCssVar/index.ts
Lines 54 to 55 in 0ec82c0
I think we probably don't need immediate because initially, both old[0] and old[1] will be undefined, so immediate: true is unnecessary.
The reason we need flush: 'post' is because at line 39 window.getComputedStyle(el).getPropertyValue(key)
vueuse/packages/core/useCssVar/index.ts
Line 39 in 0ec82c0
if you change the color value at the onMounded, since the color on the element hasn't really changed yet, we won't get the correct value here. Therefore, we need to wait for Vue to update before we get the state from the element.
There was a problem hiding this comment.
both old[0] and old[1] will be empty, so immediate: true is unnecessary.
what about updateCssVar() to get the initial value.
There was a problem hiding this comment.
Sorry, I missed this.
I believe it's also not needed because variable will always equal initialValue at the initial. It only needs to call updateCssVar when variable changes occur.
Perhaps I'm wrong.
There was a problem hiding this comment.
Sorry, I missed this.
I believe it's also not needed because
variablewill always equalinitialValueat the initial. It only needs to callupdateCssVarwhenvariablechanges occur.Perhaps I'm wrong.
I think if we dont do this immediate we might miss that one, no?
vueuse/packages/core/useCssVar/index.ts
Lines 38 to 41 in 0ec82c0
There was a problem hiding this comment.
Sorry, I missed this.
I believe it's also not needed becausevariablewill always equalinitialValueat the initial. It only needs to callupdateCssVarwhenvariablechanges occur.
Perhaps I'm wrong.I think if we dont do this immediate we might miss that one, no?
vueuse/packages/core/useCssVar/index.ts
Lines 38 to 41 in 0ec82c0
However, we have already set variable to initialValue at:
vueuse/packages/core/useCssVar/index.ts
Line 32 in 0ec82c0
In fact, in my testing, whether immediate is true or false doesn't affect the actual result. ofc, we can add immediate back first in case I missed something.
There was a problem hiding this comment.
I think this should work:
it('should have existing value', async () => {
const vm = mount(defineComponent({
setup() {
const el = ref()
const color = '--color'
const variable = useCssVar(color, el)
return {
el,
variable,
}
},
template: `
<div ref="el" style="--color: red"></div>
`
}))
await nextTick()
expect(vm.variable).toBe('red')
expect(window.getComputedStyle(vm.el).getPropertyValue("--color")).toBe('red')
expect(vm.el.style.getPropertyValue('--color')).toBe('red')
})But it does'nt. immediate does not change the test result here.
this one does remove the --color from the element.
vueuse/packages/core/useCssVar/index.ts
Lines 61 to 72 in 0ec82c0
There was a problem hiding this comment.
Oh...It seems quite tricky, thank you for noticing.
I need some time to study it.
Co-authored-by: OrbisK <37191683+OrbisK@users.noreply.github.com>
|
I believe I have completed the task. I made some minor modifications, smaller than before but still resolved the issue. Additionally, I added more tests to ensure the original behavior remains intact. Please lmk if I missed anything :) |
packages/core/useCssVar/index.ts
Outdated
| [variable, elRef], | ||
| ([val]) => { |
There was a problem hiding this comment.
Maybe not such a big difference, but I think we should add the new element here as well?
| [variable, elRef], | |
| ([val]) => { | |
| [variable, elRef], | |
| ([val, el]) => { |
I can't suggest the full function because it's not considered a change. So the function should use el too. 😄 what do you think?
There was a problem hiding this comment.
Sure, no problem, I think it's great.
|
@ilyaliao isnt this a fix of the initial value instead of new behavior? |
At first it was like this, but later I also |
resolves #4493
Before submitting the PR, please make sure you do the following
fixes #123).Description
This PR enhances several behaviors:
variabletowindow.document.documentElementor thetargeton first use, and also when thetarget elementchanges.onMountedwhen settinginitialValue.teststo ensure that both thestateand theelementare indeed changed.