fix(runtime-core): Transitions with v-if statements at root-level of components aren't working the same way as in Vue 2 #7678
Conversation
skirtles-code
left a comment
skirtles-code
left a comment
There was a problem hiding this comment.
For anyone wishing to see this running:
Suggestions from me below.
| oldInnerChild && | ||
| oldInnerChild.type !== Comment && | ||
| instance.vnode.el && | ||
| instance.vnode.el.nodeName !== '#comment' && |
There was a problem hiding this comment.
As far as I'm aware, we don't check the nodeName anywhere else in the codebase. Maybe use nodeType instead?
| template: ` | ||
| <div id="container"> | ||
| <transition name="test"> | ||
| <transition name="test" mode="out-in"> |
There was a problem hiding this comment.
Shouldn't this be added as a new test, rather than changing the existing test? They seem to be testing two separate use cases, both of which need testing.
GitHub is showing this comment against the wrong line since the code changed and it won't let me mark it as resolved.
| @@ -0,0 +1,67 @@ | |||
| <script src="../../dist/vue.global.js"></script> | |||
There was a problem hiding this comment.
I'm not exactly sure what the purpose of the examples folder is, but the other examples seem to mirror what's in the docs at https://vuejs.org/examples/. I think the point of them is to confirm that changes don't break the docs.
I'm not sure whether this fix justifies a new example. Doesn't the change to e2e/Transition.spec.ts already add the relevant test case?
|
@skirtles-code Thanks for your suggestion, I had modified my code, please review it again. Thanks again. |
|
@AlexVagrant did you see my suggestion above? |
Hi guys I see that and I had response your suggestion, did you see that? instance.vnode.type type is ConcreteComponent not VNodeTypes. |
|
@AlexVagrant you're right, i just tested it and I was wrong. Your latest version is perfect, thanks! @skirtles-code could you review this again? Would be great to get this merged soon! |
|
@himself65 may I draw your attention to this open issue with an available fix? I would love to see this merged soon. |
/cc @sxzz |
|
Soon? |
…pty root node) transition add out-in mode
yyx990803
force-pushed
the
fix/transition_v-if
branch
from
a2c2a92 to
be93185
Compare
6 participants
Fix: #7649