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: add sibling portal support & implement interact outside the right way #1080

Merged
merged 97 commits into from
May 4, 2024

Conversation

anatolzak
Copy link
Contributor

@anatolzak anatolzak commented Mar 10, 2024

closes #1077, #1153, #1167

This PR adds support for sibling portals by correctly implementing outside interactions using a stack. Previously outside interactions did not work correctly when using sibling portals.

Also, please don't be intimidated by the "size" of this PR. It's mostly tests and other than that, this PR is 70 additions and 161 deletions.

Throughout this PR, I will mention the term "floating element", what I mean by that is anything that can be mounted into a portal (i.e. dialog, tooltip, popover, link preview, menu, tooltip, listbox etc.).

The correct way to implement outside interactions is by only closing the top-most floating element given the top-most element has closeOnOutsideClick: true. If the top-most element has closeOnOutsideClick: false, no elements should close since the top-most element "asked" to not close on an outside interaction.

I also fixed an issue with the Tags Input when it's nested inside another floating element like a Popover. When editing a tag, if you click outside, we should just cancel editing. However, the popover would also close. This is solved by using the new useInteractOutside() on the edit builder to prevent other floating elements from reacting to outside interactions.

Here is a video showing nested floating elements and outside interactions only closing the top-most floating element. (ignore the craziness of the nested elements :))

Screen.Recording.2024-03-15.at.4.55.22.PM.mp4

Also, to prevent issue #1167, where interact outside doesn't fire when clicking outside opens a new floating element, which is caused by checking too late whether the element is at the top of the stack. This is because the outside interaction triggers the opening of another floating element, so by the time pointerup fires for the first floating element, the second floating element had already been added to the stack. This is prevented in this PR by checking in pointerup whether the element was at the top of the stack during pointerdown. I added a regression test for this as well.

Before

Screen.Recording.2024-04-08.at.1.28.18.PM.mov

After

Screen.Recording.2024-04-08.at.1.48.51.PM.mov

Copy link

changeset-bot bot commented Mar 10, 2024

🦋 Changeset detected

Latest commit: 01c0f8b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@melt-ui/svelte Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@huntabyte
Copy link
Member

Oh no not the timezone tests failing due to DST again! I'll get that fixed later today on the develop branch so you can merge it in!

@huntabyte
Copy link
Member

That failing test may just need an await waitFor() for the focus. If it still doesn't pass then it may have been introduced by this PR

@anatolzak
Copy link
Contributor Author

@huntabyte not sure why the tests are not passing. I tried to manually test the functionality that's not passing the tests and at first glance, things seem to be working correctly. Will look into it tomorrow

@anatolzak
Copy link
Contributor Author

@huntabyte it seems like merging the following PR will fix the tests for Date Picker #1055
it looks like the root issue was with the popover component which the Date Picker uses

@alexbjorlig
Copy link

Hi everyone, the work here is amazing 🚀 Just wanted to let you all know that in 21RISK we are also facing this issue, so it will be so good when it's fixed.

If there is anything we can help with let us know.

@huntabyte
Copy link
Member

Hey @alexbjorlig, thanks for your patience! There are a lot of small (but mighty) internal changes going on simultaneously, so we're doing our best to ensure we don't introduce a regression into the library!

Copy link
Member

@TGlide TGlide left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Amazing PR as always @anatolzak 🎉

@TGlide TGlide merged commit 5a2d945 into melt-ui:develop May 4, 2024
4 of 6 checks passed
@anatolzak anatolzak deleted the feat/add-sibling-portal-support branch May 4, 2024 14:54
@github-actions github-actions bot mentioned this pull request May 4, 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.

Interact Outside Issue When Mounting Tooltip in Popover in separate portals
4 participants