Skip to content
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

fix(runtime-core): Transitions with v-if statements at root-level of components aren't working the same way as in Vue 2 #7678

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

AlexVagrant
Copy link
Contributor

Fix: #7649

Copy link
Contributor

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone wishing to see this running:

Suggestions from me below.

@@ -224,6 +224,8 @@ const BaseTransitionImpl: ComponentOptions = {
if (
oldInnerChild &&
oldInnerChild.type !== Comment &&
instance.vnode.el &&
instance.vnode.el.nodeName !== '#comment' &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I'm aware, we don't check the nodeName anywhere else in the codebase. Maybe use nodeType instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@@ -1148,7 +1148,7 @@ describe('e2e: Transition', () => {
createApp({
template: `
<div id="container">
<transition name="test">
<transition name="test" mode="out-in">
Copy link
Contributor

@skirtles-code skirtles-code Feb 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@AlexVagrant
Copy link
Contributor Author

@skirtles-code Thanks for your suggestion, I had modified my code, please review it again. Thanks again.

@lehni
Copy link
Contributor

lehni commented Mar 21, 2023

@AlexVagrant did you see my suggestion above?

@AlexVagrant
Copy link
Contributor Author

@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.

@lehni
Copy link
Contributor

lehni commented Mar 21, 2023

@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!

@lehni
Copy link
Contributor

lehni commented Apr 11, 2023

@himself65 may I draw your attention to this open issue with an available fix? I would love to see this merged soon.

@himself65
Copy link
Contributor

@himself65 may I draw your attention to this open issue with an available fix? I would love to see this merged soon.

/cc @sxzz

@luckydonald
Copy link

luckydonald commented May 29, 2024

Soon?

@yyx990803 yyx990803 merged commit ef2e737 into vuejs:main Jun 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Transitions with v-if statements at root-level of components aren't working the same way as in Vue 2
6 participants