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(transition): reflow before leave-active class after leave-from (#2593), while fixing (#10688) #12288

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tadehz
Copy link

@Tadehz Tadehz commented Oct 29, 2024

Fixes an old issue (#2593) that was introduced again by commit 65109a7 while retaining the new functionality of the fixed issue (#10688).

Maybe the v-leave-from class should just not be added at all if the enter transition is cancelled? This should be more looked into.

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB (+77 B) 38 kB (+26 B) 34.2 kB (+17 B)
vue.global.prod.js 159 kB (+77 B) 57.9 kB (+26 B) 51.5 kB (+29 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 46.9 kB 18.3 kB 16.7 kB
createApp 55 kB 21.3 kB 19.4 kB
createSSRApp 59 kB 23 kB 20.9 kB
defineCustomElement 59.8 kB 22.8 kB 20.8 kB
overall 68.8 kB (+103 B) 26.4 kB (+41 B) 24 kB (+12 B)

@edison1105
Copy link
Member

Could you please add a test case for this to avoid regression?

Copy link

pkg-pr-new bot commented Oct 29, 2024

Open in Stackblitz

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@12288

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@12288

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@12288

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@12288

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@12288

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@12288

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@12288

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@12288

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@12288

vue

pnpm add https://pkg.pr.new/vue@12288

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@12288

commit: 237e5e9

@edison1105 edison1105 added the need test The PR has missing test cases. label Oct 29, 2024
@edison1105
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Oct 29, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools success success
nuxt success success
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify success success
vueuse success success
vue-simple-compiler success success

@Tadehz
Copy link
Author

Tadehz commented Oct 29, 2024

Could you please add a test case for this to avoid regression?

Not entirely sure how to go about it, but I will try.

@Tadehz
Copy link
Author

Tadehz commented Oct 30, 2024

I'm not sure if this is really the best way, because it required me to modify the transition.html file and add a new function in e2eUtils.ts. I couldn't think of an easier and less intrusive way to check for reflow reliably. Maybe it's possible to do so with just classes, but I tried that multiple times with no success.

@edison1105
Copy link
Member

edison1105 commented Oct 31, 2024

I'm not sure if this is really the best way, because it required me to modify the transition.html file and add a new function in e2eUtils.ts. I couldn't think of an easier and less intrusive way to check for reflow reliably. Maybe it's possible to do so with just classes, but I tried that multiple times with no success.

LGTM.
Since #10677 does not have a new test case, it would be nice to add one too.

@edison1105 edison1105 added ready to merge The PR is ready to be merged. 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. and removed need test The PR has missing test cases. labels Oct 31, 2024
@Tadehz
Copy link
Author

Tadehz commented Nov 2, 2024

I'm not sure if this is really the best way, because it required me to modify the transition.html file and add a new function in e2eUtils.ts. I couldn't think of an easier and less intrusive way to check for reflow reliably. Maybe it's possible to do so with just classes, but I tried that multiple times with no success.

LGTM. Since #10677 does not have a new test case, it would be nice to add one too.

Just adding a few lines into the new test actually would be enough, I think, but I'm not sure if the current behavior is really the best, because right now the v-leave-from class will not give any visual results without a reflow in case of a cancelled enter transition, so I would say it's redundant. And since there is a dedicated enter-cancelled event, it's very easy to add the leave-from class manually in edge cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready to merge The PR is ready to be merged. scope: transition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants