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: use stacks in useEscapeKeydown & introduce escapeBehavior prop #1142

Merged
merged 40 commits into from
Apr 21, 2024

Conversation

anatolzak
Copy link
Contributor

@anatolzak anatolzak commented Mar 30, 2024

closes #1141, #1143

This PR fixes how we handle escape presses when dealing with multiple nested floating elements by maintaining a stack of the open elements. This was part of #1080 but I realized that it changed too many things and would make it unnecessarily difficult to review.

Here is a video showing nested floating elements and outside escape presses only closing the top-most floating element.

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

Previously we added closeOnEscape to the dependencies in the effect of every builder that was interested in the escape behavior. This breaks as soon as we use stacks, since updating closeOnEscape while an element is open removes the element from stack and then adds it to the top of the stack. To prevent that, this PR allows passing the closeOnEscape store to useEscapeKeydown which therefore allows updating closeOnEscape without reseting the position of the element in the stack. I added tests for this as well.

Now that we use a stack to keep track of open elements that are listening to the escape presses, I had to remove a lot of extra keydown event listeners that we had in many builders that would interfere when using multiple nested elements.

This PR also replaces the closeOnEscape builder prop with escapeBehavior which provides even further fine-grained control over the escape behavior in builders.

Previously when dealing with multiple nested floating elements and passing closeOnEscape: false to a child element, when the escape key was pressed, both the parent and the child would close. I would expect neither the parent nor the child to close since I explicitly "asked" the child not to close on escape. So previously there was no way to force an element to stay open when escape is pressed without triggering the parent to close as well.

To allow more fine-grained behaviors, we now introduce the escapeBehavior prop:

  • close: Closes the element immediately.
  • ignore: Prevents the element from closing and also blocks the parent element from closing in response to the Escape key.
  • defer-otherwise-close: Delegates the action to the parent element. If no parent is found, it closes the element.
  • defer-otherwise-ignore: Delegates the action to the parent element. If no parent is found, nothing is done.

Here are videos showing the behavior of adding different escapeBehavior values to a popover that is nested inside a dialog. The dialog's escapeBehavior is close.

Popover escapeBehavior: 'close'

Screen.Recording.2024-03-31.at.5.51.58.PM.mov

Popover escapeBehavior: 'ignore'

Screen.Recording.2024-03-31.at.5.52.21.PM.mov

Popover escapeBehavior: 'defer-otherwise-ignore'

Untitled.mov

Popover escapeBehavior: 'defer-otherwise-close'

Screen.Recording.2024-04-05.at.5.21.40.PM.mov

Popover escapeBehavior: 'defer-otherwise-close' mounted as a single element

Screen.Recording.2024-04-05.at.5.22.04.PM.mov

Copy link

changeset-bot bot commented Mar 30, 2024

🦋 Changeset detected

Latest commit: 63d3110

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

@anatolzak anatolzak changed the title fix(escape-keydown) correctly handling escape keydown with nested elements fix(escape-keydown) handle correctly escape keydown with nested elements Mar 30, 2024
@anatolzak anatolzak marked this pull request as draft March 30, 2024 15:39
@anatolzak anatolzak marked this pull request as ready for review March 31, 2024 14:26
@anatolzak anatolzak changed the title fix(escape-keydown) handle correctly escape keydown with nested elements feat(escape-keydown): use stacks & introduce escapeBehavior Apr 1, 2024
@anatolzak anatolzak changed the title feat(escape-keydown): use stacks & introduce escapeBehavior feat(escape-keydown): use stacks & introduce escapeBehavior prop Apr 1, 2024
@anatolzak anatolzak changed the title feat(escape-keydown): use stacks & introduce escapeBehavior prop Feat: use stacks in useEscapeKeydown & introduce escapeBehavior prop Apr 5, 2024
Copy link
Contributor

github-actions bot commented Apr 21, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
melt-ui ✅ Ready (View Log) Visit Preview 63d3110

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.

Dude. this is amazing! Such a comprehensive PR. Thank you!

@TGlide TGlide merged commit f15cd30 into melt-ui:develop Apr 21, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Apr 21, 2024
@anatolzak anatolzak deleted the fix/escape-keydown-nested-elements branch April 21, 2024 09:32
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.

🌟Feature request: Add more options to closeOnEscape prop
2 participants