-
Notifications
You must be signed in to change notification settings - Fork 35
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: added nav menu item close on click outside #819
base: dev
Are you sure you want to change the base?
feat: added nav menu item close on click outside #819
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @antoniosarro, thank you for taking the time to work on this feature.
I found one bug and a few other things that I think would improve upon the code. Let me know if there's anything I can help with further or if I am misunderstanding anything in my comments.
Thanks!
@@ -222,7 +222,10 @@ | |||
{/if} | |||
</button> | |||
{#if detailedMenuShown} | |||
<DetailedMenu /> | |||
<DetailedMenu | |||
clickOutsideCallback={() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be a bug introduced with each Menu that uses this callback. You can no longer press the menu button used to open the menu to close it.
It seems that when you try to close a menu with its menu button in the nav (using the sort menu as an example, happens to all):
- The
clickOutsideCallback
will setsortMenuShown
tofalse
- The menu button
onclick
will setsortMenuShown
back totrue
. - Menu stays open and cannot be closed via the initial button used to open it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the fastest and simplest method to solve this bug is to add a class to the button that opens the menu, and exclude clicks on that particular class within the action. Tell me what you think and I'll take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the fastest and simplest method to solve this bug is to add a class to the button that opens the menu, and exclude clicks on that particular class within the action. Tell me what you think and I'll take a look
It seems like that may be the only solution here. I think it would force us to use different ids for each button and pass them down to each menu individually as to not break clicking on a different nav menu button to switch over to it. Do you think it could work like that (if necessary)?
We should probably add unique id
s to the buttons in a certain format (like: id="menuButtonNavSort").
I think we'd also have to check if the click target was an element inside of the menu button (eg: the icon; an svgs <path>
)
I made the changes and it seems to work as it should, I don't know if it is necessary to define the id used to exclude buttons as a const somewhere so that it is easier to modify it in case for each occurrence. |
Hi @antoniosarro, thanks for making them changes. Looks like I am seeing a new bug: Clicking into the nav or body just below the nav still closes to popup, but for some reason clicking anywhere in the posters wrapper or on posters doesn't close the popup (eg: space around posters, poster rating/status button). Same on other pages too, just the top part of the page will close them, nothing in the actual body. |
Yes, I saw that it was a bug that I had missed because when I tested it the poster wrapper was not present. |
Navbar menus now close when anywhere outside the menu is clicked.
Related to issue #813