Open sviripa opened 4 months ago
Latest commit: b37659a1cef67f80dd8f75f2c524ab2778a8e20d
The changes in this PR will be included in the next version bump.
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
Name | Status | Preview | Last Commit |
---|---|---|---|
runed | ✅ Ready (View Log) | Visit Preview | b37659a1cef67f80dd8f75f2c524ab2778a8e20d |
@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?
@TGlide I'll refactor this one, no worries!
Hey @sviripa, are you still planning to refactor this, or should one of us push it over the finish line?
@huntabyte @TGlide I just pushed the changes to make useClickOutside
to use the new internals. Sorry for the delay
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
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.
@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
@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:
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()
.
Also, you may edit your original comment to Closes #37
as this will trigger github to auto close the issue when this is merged.
@Hugos68 Thanks for your comments and examples 🤝
@TGlide Please take a look when you have time
This PR implements
useClickOutside
requested in #37