Details refactor (non-breaking) #429
Conversation
| function openMenu() { | ||
| if (!open) { | ||
| setOpen(true) | ||
| document.addEventListener('click', closeMenu) |
There was a problem hiding this comment.
I'm trying to think of why this might not need to be qualified with listener options, namely:
{capture: true}so that other elements' click handlers can't prevent this one?{once: true}to ensure that it's only called once?
But maybe we leave it as-is for now and revisit when we've used it more? /cc @muan who knows way too much about this subject 😄
There was a problem hiding this comment.
I don't really understand the premise here 😬. From the original issue body it sounds like those menus just shouldn't use the optional overlay?
But looks like this will also close details when user clicks inside the menu which wasn't the case.
There was a problem hiding this comment.
The reason why I didn't want to use the CSS overlay is that when there is a menu open, and I want to click on another menu to open the new menu, clicking only closes the first menu, and doesn't open the second one (the second menu's click never triggers because it's behind the overlay)
There was a problem hiding this comment.
And yeah, we do want to close the menu when the user clicks inside of a menu because that will jump them down to a different part of the page and at that point the menu can/should be closed
There was a problem hiding this comment.
Let me see if I can get deploys working so that y'all can play around with it
There was a problem hiding this comment.
In dotcom there is a JS class that listens to <details>'s toggle open event, and close other <details open> with the same class name; it can be used with or without .details-overlay.
When used with .details-overlay, the only way to get into that state is with keyboard, which was the use case the class was initially made for.
There was a problem hiding this comment.
Hmm yeah, I think our current assumption is that any click inside of the menu closes it because it's a link or button. 🤔
This is a test of the retry functionality in primer/deploy#12
This PR refactors the Details element to use a document event listener to show/hide the Details element when a click is made, instead of an overlay element. The overlay element works fine if you've only got one Details element on the page, but if you're using several (in the case of the new docs header), clicking on another Details element doesn't open the new element, because the overlay is blocking the DOM element.
I wrote this to prioritize not breaking the current API of the Details element, which is why there's a conditional in the
togglefunction now. I felt this was a fair compromise to keep the API the same :)