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 useClickOutside #46

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

Conversation

sviripa
Copy link

@sviripa sviripa commented May 3, 2024

This PR implements useClickOutside requested in #37

Copy link

changeset-bot bot commented May 3, 2024

🦋 Changeset detected

Latest commit: b37659a

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

This PR includes changesets to release 1 package
Name Type
runed 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

Copy link
Contributor

github-actions bot commented May 3, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
runed ✅ Ready (View Log) Visit Preview b37659a

@TGlide
Copy link
Member

TGlide commented Jun 1, 2024

@sviripa our internals have changed quite a bit since this PR. Sorry for the long time to review!

Would you like to refactor it (maybe to a class too), or should I close it?

@sviripa
Copy link
Author

sviripa commented Jun 3, 2024

@TGlide I'll refactor this one, no worries!

@huntabyte
Copy link
Member

Hey @sviripa, are you still planning to refactor this, or should one of us push it over the finish line?

@sviripa
Copy link
Author

sviripa commented Jul 2, 2024

@huntabyte @TGlide I just pushed the changes to make useClickOutside to use the new internals. Sorry for the delay

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.

Some small changes needed :)

@Hugos68
Copy link
Collaborator

Hugos68 commented Jul 2, 2024

Just wanted to add that the current method of node.contains(otherNode) does not work for the native <dialog> element which is often used for the utility. I suggest we use a different method of checking wether or not the click was inside the target element:

const rect = event.target.getBoundingClientRect();
const clickedInside = rect.top <= event.clientY && event.clientY <= rect.top + rect.height && rect.left <= event.clientX && event.clientX <= rect.left + rect.width;

if (clickedInside) {
    callback();
}

On top of that we probably want to add the following guard clause:

if (event.target.tagName !== 'DIALOG') {
    return;
}

As this removes issues with form inside dialogs.

This code comes from a gist I often use for click outside functionality: https://gist.github.com/Hugos68/27376946bfd21f431a0ee395f1e5ad71

@Hugos68
Copy link
Collaborator

Hugos68 commented Jul 2, 2024

I also think we should use document for event listeners like click instead of window as document catches the click event just as good and is closer to the source in the DOM tree so it makes more sense.

@sviripa
Copy link
Author

sviripa commented Jul 4, 2024

@Hugos68 could you please provide an example of how node.contains approach does not work for dialog tags? I tested the utility with the dialog tag locally and it behaved as expected

@Hugos68
Copy link
Collaborator

Hugos68 commented Jul 4, 2024

@sviripa Sure! REPL

In the REPL I open the dialog by default for demo purposes and when you try to close it you'll see "inside dialog!" get logged.
This is because the ::backdrop isn't a seperate element but actually a pseudo element of the dialog element. To solve this we need to check the rect of the dialog element so we can distinguish if a click was actually on the element or one of it's pseudo elements.

Here is a visualisation I just made:
image

Currently this issue only exists for <dialog> elements but I imagine as the web progresses more pseudo elements are added that could interefere with node.contains click outside detection, so IMO it's safer bet using getBoundingClientRect().

@Hugos68
Copy link
Collaborator

Hugos68 commented Jul 5, 2024

Also, you may edit your original comment to Closes #37 as this will trigger github to auto close the issue when this is merged.

@sviripa
Copy link
Author

sviripa commented Jul 21, 2024

@Hugos68 Thanks for your comments and examples 🤝

@TGlide Please take a look when you have time

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.

4 participants