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

feat(Slideovers): Allow multiple instances of programmatic slideover #1758

Closed
wants to merge 25 commits into from

Conversation

genu
Copy link
Contributor

@genu genu commented May 7, 2024

πŸ”— Linked issue

❓ 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

This PR opens the possibility to open multiple instance of the <Slideover/> component using the useSlideover() composable.

This capability was discussed in the past, so this is an attempt at such a feature.

πŸ“ Checklist

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

@genu
Copy link
Contributor Author

genu commented May 7, 2024

Tagging @noook for feedback

@genu genu changed the title Feature/multi slideover Feature: Allow Multiple instances of programmatic slideover May 7, 2024
@genu genu changed the title Feature: Allow Multiple instances of programmatic slideover Feature: Allow multiple instances of programmatic slideover May 7, 2024
src/runtime/composables/useSlideover.ts Show resolved Hide resolved
src/runtime/types/slideover.d.ts Show resolved Hide resolved
src/runtime/composables/useSlideover.ts Outdated Show resolved Hide resolved
src/runtime/composables/useSlideover.ts Outdated Show resolved Hide resolved
src/runtime/composables/useSlideover.ts Outdated Show resolved Hide resolved
src/runtime/components/overlays/Slideovers.client.vue Outdated Show resolved Hide resolved
src/runtime/composables/useSlideover.ts Outdated Show resolved Hide resolved
@genu genu force-pushed the feature/multi-slideover branch from 74aee7b to 7253ac5 Compare May 9, 2024 02:42
@noook
Copy link
Collaborator

noook commented May 10, 2024

There is an issue: Slideovers should be dismissable when clicking outside unless you explicitly prevent them from closing with the appropriate prop.

Your example components do not specify it so it should keep the original behaviour:
https://ui-9x1er5zyw-nuxt-js.vercel.app/dev/components/slideover#control-programmatically

@genu genu changed the title Feature: Allow multiple instances of programmatic slideover feat(Slideovers): Allow multiple instances of programmatic slideover May 13, 2024
The code changes in `Slideovers.client.vue` remove the unused `<pre>` element that displayed the `slideoverInstances` props. This cleanup improves code readability and removes unnecessary code.
@genu
Copy link
Contributor Author

genu commented May 13, 2024

There is an issue: Slideovers should be dismissable when clicking outside unless you explicitly prevent them from closing with the appropriate prop.

Your example components do not specify it so it should keep the original behaviour: https://ui-9x1er5zyw-nuxt-js.vercel.app/dev/components/slideover#control-programmatically

I fixed it by listening to model changes on the modal and closing it when the model is changed. An issue I run into is that when clicking on the outside, it always closes the first one that was open, rather than the last one (i.e. the one on top).

This issue would probably happen if we implement something similar in UModal. I'm digging through how the outside click is handled in the component and see if there is anything to be done there.

@noook Open to suggestions.

@benjamincanac
Copy link
Member

Would it be possible to apply the same changes to the useModal composable in the same PR? @noook What do you think?

# Conflicts:
#	docs/components/content/examples/SlideoverExampleComponent.vue
@genu
Copy link
Contributor Author

genu commented Aug 23, 2024

@benjamincanac I updated this PR to move it along.

A caveat that I ran into, (maybe its not a major issue?) is the preventClose attribute (which defaults to false) affects all of the slideover instances.

What this means in practice is that when preventClose is set to false, all of the slideovers on the page would respond to the close event, becaues there is no way to know "which" slideover's backdrop actually triggered the close event.

I don't think there is a way to get around this limitation at the moment without doing some gymnastics around the headlessui components.

We can document this limitation and roll with it, or wait until the next major release and deal with it then when we move to radix-vue

Let me know your thoughts.

If we decide to roll with it, I can make the same changes to the useModal composable.

@benjamincanac
Copy link
Member

@genu Sorry for the delay!! Should we really release this with the Headless UI limitation though?

I would rather implement this only in the v3.

@genu
Copy link
Contributor Author

genu commented Sep 17, 2024

I agree, not a big fan releasing a half baked solution.

I'm fine with revisiting this feature with the release of v3.

As a side note, I have been playing with vue-final-modal as a drop replacement for both useModal and useSlideover and it's been working much better than the built in compostables.

Perhaps for v3, it can be used as an inspiration.

Copy link
Member

Should we close this then? If you want to work on this, you can make a pull request against the v3 branch already https://github.com/nuxt/ui/tree/v3 😊

@genu genu closed this Sep 18, 2024
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.

3 participants