Skip to content

Conversation

clairep94
Copy link
Collaborator

@clairep94 clairep94 commented Sep 3, 2025

pr05 Typescript Migration 8: Migrate the client/components/Menubar/MenubarSubmenu

Context:

Changes:

  • Menubar/MenubarSubmenu
  • Menubar/contexts

Other:

  • any updates to use named exports

Notes:

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@clairep94 clairep94 added the pr05 Grant Projects pr05 Grant Projects label Sep 3, 2025
@clairep94 clairep94 changed the title Pr05/migrate client components final rebuild menubar submenu attempt 2 pr05 Typescript #8: migrate client/components/Menubar/MenubarSubmenu Sep 3, 2025
MENU = MenubarListItemRole.MENU,
LISTBOX = MenubarListItemRole.LISTBOX,
TRUE = 'true'
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khanniie I'm not 100% if this was the correct approach

I was a bit confused because the original file had the below:

MenubarTrigger.propTypes = {
  role: PropTypes.string,
  hasPopup: PropTypes.oneOf(['menu', 'listbox', 'true'])
};

MenubarList.propTypes = {
  children: PropTypes.node,
  role: PropTypes.oneOf(['menu', 'listbox'])
};

I made the assumption the menu bar list item roles and menubar trigger aria-hasPopup are associated in the relationship defined in the enums??
Should we just remove TRUE? I didn't see TRUE being used elsewhere

const { isOpen, handlers } = useMenuProps(id);

// `ref` is always a button from MenubarSubmenu, so safe to cast.
const buttonRef = ref as React.RefObject<HTMLButtonElement>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't sure how to resolve this other than casting, but I thought was safe bc this function is only used within this file once and it's always a button ref

children,
id,
title,
triggerRole: customTriggerRole,
listRole: customListRole,
triggerRole = 'menuitem',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if string is too loose here, or if there is a specific enum we should be using?

@clairep94 clairep94 marked this pull request as ready for review September 3, 2025 18:42
@clairep94
Copy link
Collaborator Author

@raclim @khanniie ready for review! this is build off of PR 7, which was big so might be best to wait till PR 7 is merged to look at this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr05 Grant Projects pr05 Grant Projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant