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(Modal): wait for transition to complete to reset state #1618

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

noook
Copy link
Collaborator

@noook noook commented Apr 4, 2024

πŸ”— Linked issue

Fixes #1557

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

When using modals programmatically, a few error happen:

  • Modal state is kept after closing the modal
  • When closing the modal with the composable API, the state is reset too early, leading to the component being unmounted before the transition is complete.

To address these issues, we listen to the "after-leave" event fired by Headless UI's transition, so that we know it is safe to unmount the component.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@noook noook force-pushed the fix/use-modal-improvements branch from d37e274 to f5eb90e Compare April 4, 2024 21:10
@noook
Copy link
Collaborator Author

noook commented Apr 4, 2024

Btw @genu , I think you can port this on useSlideover as well

@genu
Copy link
Contributor

genu commented Apr 4, 2024

Thanks, I will take a look

@benjamincanac benjamincanac changed the title fix(Modal): Wait for transition to complete to reset state fix(Modal): wait for transition to complete to reset state Apr 5, 2024
@benjamincanac
Copy link
Member

@noook Sorry for the conflicts, I've made some cleaning in #1619.

@benjamincanac benjamincanac merged commit 2bdb5d2 into nuxt:dev Apr 5, 2024
2 checks passed
@genu
Copy link
Contributor

genu commented Apr 5, 2024

@noook In testing it useSlideover I'm not seeing any race conditions during the transitions.

I can make this change, but since the issue doesn't exist, I'm hesitant to add a fix for a non-existent problem.

Am I missing something?

@noook
Copy link
Collaborator Author

noook commented Apr 5, 2024

Hmm, maybe it doesn't happen. To make it short what happened with the modals when closing programmatically is that the reset was done before the transition was complete, which led to the transition being shorter as there is remaining component

If you don't see any issues on your side then it's fine I guess

@genu
Copy link
Contributor

genu commented Apr 5, 2024

I'll continue testing on my end and keep an eye out for issues that come up in the meantime.

@genu
Copy link
Contributor

genu commented Apr 5, 2024

I take that back, I think I had some wonky caching issues before. I do see the issue as well in useSlideover

I'll get a PR ready for fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening/closing modals programmatically
3 participants