I think the only issue I might've found is that I noticed that when using the arrow keys to navigate specifically the File SubMenu, it ends up skipping over "Open".

ooo interesting ... for some reason this didn't happen to me earlier but now it's occurring. it also gets very weird when I save the sketch and new menu items appear? seems like it has something to do with hideIf prop. the way I'm tracking the menu items probably isn't accounting for the conditionally rendered options. definitely needs another look.

separately, i also run into an error when the user is unauthenticated and attemps to click "Login" or "Signup", as it's looking for a menuitem element to focus but there isn't one in this case.

1. When navigating the SubMenus, I noticed that both the mouse hover and arrow key inputs are visible, which I tried to depict in the video. I feel like this could get a bit confusing, but unsure if it might've been intentional!

definitely not intentional, should probably prioritize one interaction over the other!

2. I also noticed that the focus state immediately appears on the Menubar as soon as I landed on the editor page. I was wondering if this might've been intended as well!

i should fix this as well, the auto-focusing feels disruptive and likely isn't wcag-oriented.

3. I know there's a few different opinions around adding comments, I was curious about why you decided to add more explanatory comments throughout the components!

thanks for this question! most comments I added for myself just to keep track of what was happening as it was getting a little complex ... i also felt that it wasn't always clear what some code was doing. i can probably remove some of the comments for code that is more self-evident.

4. While looking through `MenubarSubmenu.jsx`, I feel like it could be broken down into maybe a few files, but was wondering if you felt that way as well!

yup, agreed! this is a change that makes sense to me.

i know it's a lot to review so thanks for taking the time! let me go back in and address some of the more urgent bugs regarding the auto-focusing and weird navigation. as for the other issues (mouse and key inputs being visible, breaking down the submenu into smaller files, etc ...) do you think it's ok if i add those to this PR or can I address them in future ones?

i know it's a lot to review so thanks for taking the time! let me go back in and address some of the more urgent bugs regarding the auto-focusing and weird navigation. as for the other issues (mouse and key inputs being visible, breaking down the submenu into smaller files, etc ...) do you think it's ok if i add those to this PR or can I address them in future ones?

That sounds good to me!! I think the other issues can probably be addressed in future ones, but please feel free to go ahead with whichever feels best to you! Thanks so much again for your efforts on this! :)

update: part of the navigation bug looks related to the early return in MenubarItem where we return null if hideIf is true. i moved the registration logic above that and added a check to only register if the menuitem is visible.

this seems to work but breaks again if the user refreshes the page. might need to dig a little deeper into this ...

When navigating the SubMenus, I noticed that both the mouse hover and arrow key inputs are visible, which I tried to depict in the video. I feel like this could get a bit confusing, but unsure if it might've been intentional!

Here's one way it could work: https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/

It only ever highlights a single item at a time, which follows the keyboard navigation, but jumps to a different element on mouse hover. Maybe this is not very easy to implement and requires additional logic.

Another option could be to have different styles on hover and focus, since they have different semantics (eg. pressing enter will select the focused option, not the hovered one)

@dipamsen thanks for linking the example! that's been the model for this menubar's implementation and is aligned with other examples i've seen -- i'll be using the same approach.

@raclim spent a little more time with the navigation issue. i think the problem is in the menuitems which get conditionally rendered; there seems to be a conflict between when the menuitems are registered and when the conditions for hideIf change. hideIf tends to change after the component mounts, so when the submenu item eventually registers, it gets added to the end of the submenuItems set. even if they are rendered a different way they still get navigated to as if they were the last items. e.x. if a user signs in, saves the sketch, then refreshes the page, the items which depend on isUnsaved (Duplicate, Share, etc) are added at the end of the navigation order.

i'm curious how you'd feel if, instead of conditionally rendering the items, maybe keeping them visible but disabled instead? that way the ordering of the menuitems can be stable regardless of visibility. i know that the example menubars we looked at above do something similar as well, depending on the context, so it looks like an established pattern. it will probably be an adjustment though, so i just wanted to check in, in case there was an argument for keeping the conditional rendering.

here's how things look with the disabled but visible approach.

unsaved, unauthenticated

unsaved, authenticated

saved, authenticated

i used some existing inactive styles. if we go this route we should probably push it more since it's still hard to tell if an item is disabled or not. navigation works as expected too. we can add tooltips or modals to clarify why an item is disabled.

spent a little more time with the navigation issue. i think the problem is in the menuitems which get conditionally rendered; there seems to be a conflict between when the menuitems are registered and when the conditions for hideIf change. hideIf tends to change after the component mounts, so when the submenu item eventually registers, it gets added to the end of the submenuItems set. even if they are rendered a different way they still get navigated to as if they were the last items. e.x. if a user signs in, saves the sketch, then refreshes the page, the items which depend on isUnsaved (Duplicate, Share, etc) are added at the end of the navigation order.

Thanks so much for catching this—that explains a lot! It makes so much sense when I interact with the menubar now!!! 😂

i'm curious how you'd feel if, instead of conditionally rendering the items, maybe keeping them visible but disabled instead? that way the ordering of the menuitems can be stable regardless of visibility. i know that the example menubars we looked at above do something similar as well, depending on the context, so it looks like an established pattern. it will probably be an adjustment though, so i just wanted to check in, in case there was an argument for keeping the conditional rendering.

Hmm, yeah since disabling the menuitems seems like an established pattern, I'm totally on board with going that route here! I think the conditional rendering was originally used because the component was first built using toggleDropdown to control when the menu items would render.

i used some existing inactive styles. if we go this route we should probably push it more since it's still hard to tell if an item is disabled or not. navigation works as expected too. we can add tooltips or modals to clarify why an item is disabled.

Thanks so much for linking the screenshots and confirming that it seems to work! I'm wondering do you think it makes sense to break this out into a separate PR, or is it better to keep it within this one?

Thanks so much for linking the screenshots and confirming that it seems to work! I'm wondering do you think it makes sense to break this out into a separate PR, or is it better to keep it within this one?

i thought i'd want to break it out, but i think it makes more sense to keep in the current PR since it feels necessary for the component to work. i'll keep the changes small we can open up another PR in the future in case we need to make improvements.