-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
pr05 Typescript #8: migrate client/components/Menubar/MenubarSubmenu #3623
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
pr05 Typescript #8: migrate client/components/Menubar/MenubarSubmenu #3623
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.
@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
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.
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
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.
not sure if string is too loose here, or if there is a specific enum we should be using?
...ild__MenubarSubmenu_attempt_2
Uh oh!
There was an error while loading. Please reload this page.
pr05 Typescript Migration 8: Migrate the client/components/Menubar/MenubarSubmenu
Context:
Changes:
Other:
Notes:
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123