[docs-infra] Fix product selector popup not closing on route change #41166
Conversation
Netlify deploy previewhttps://deploy-preview-41166--material-ui.netlify.app/ Bundle size report |
oliviertassinari
added
the
PR: needs revision
danilo-leal
changed the title
danilo-leal
added
bug 🐛
There was a problem hiding this comment.
I'm not super confident with the idea of check if element is a link, or it's parent is a link to handle that.
Did you consider the approach
let link = event.target.closest('a'); // get closest link
if (!link) return; // was not link
if (!event.currentTarget.contains(link)) return; // the link is outside| @@ -54,6 +55,18 @@ function ProductDrawerButton(props) { | |||
| setAnchorEl(null); | |||
| }; | |||
|
|
|||
| const handleEventDelegation = (e) => { | |||
| // In case of key down events, and any key apart from `enter` is clicked then don't close the menu. | |||
| if (e?.type === 'keydown' && e.keyCode !== 13) { | |||
There was a problem hiding this comment.
We avoid the use of keyCode. See this PR message for more context: #22569
There was a problem hiding this comment.
By the way, Enter should also trigger onClick so no need to add this kind of verification
There was a problem hiding this comment.
Hey @alexfauquette I have moved to useing .key instead. This check is just to ensure that whenever links are moved through keyboard and clicked on using 'enter', but without the handling it works too. So removed unnecessary code.
| @@ -54,6 +55,15 @@ function ProductDrawerButton(props) { | |||
| setAnchorEl(null); | |||
| }; | |||
|
|
|||
| const handleEventDelegation = (e) => { | |||
| // Assert whether an 'a' tag resides in the parent of the clicked element through which the event bubbles out. | |||
| const isLinkInParentTree = e?.target ? Boolean(e.target.closest('a')) : false; | |||
There was a problem hiding this comment.
In which case would you get the event undefined?
| const isLinkInParentTree = e?.target ? Boolean(e.target.closest('a')) : false; | |
| const isLinkInParentTree = Boolean(event.target.closest('a')) ; |
There was a problem hiding this comment.
Makes sense, I was checking the event handler type and thought that target is optional there but it wasn't. Done the other change too.
| @@ -54,6 +55,15 @@ function ProductDrawerButton(props) { | |||
| setAnchorEl(null); | |||
| }; | |||
|
|
|||
| const handleEventDelegation = (e) => { | |||
There was a problem hiding this comment.
| const handleEventDelegation = (e) => { | |
| const handleEventDelegation = (event) => { |
just for consistency with the rest of the codebase
fixes #41128
Description
The issue arose from the fact that on route changes since the layout is not rendered but the content panel is, in some cases when previous route was cached the popper never closed on clicking a link in it. Now, we track for route changes in the component, another approach could've been to send
handleClosethrough props but it would need special handling when new elements are added in the popper.