-
Notifications
You must be signed in to change notification settings - Fork 794
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
fix: a11y status for menu button #4313
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey there @alisonjoseph the issue for wrong menu button github link is that in storybook its "Menu button" but in carbon website its "Menu buttons" which is making incorrect github url. For now I fixed it with a hack because I was not sure renaming Menu buttons as "Menu button" as it can break other things too in carbon website !! what do you think ? |
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 we could just pull in "Menu button" directly vs. adding custom logic here. Also I think it would be good to add the additional components to the Usage page a11y status cards.
<A11yStatus layout="table" components="Menu buttons" /> | ||
<A11yStatus | ||
layout="table" | ||
components={['Menu buttons', 'Combo button', 'Overflow menu']} |
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.
components={['Menu buttons', 'Combo button', 'Overflow menu']} | |
components={['Menu button', 'Combo button', 'Overflow menu']} |
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 already did that but there is no component called menu button so its not even getting rendered in the table thats what I was not sure about either I should rename to component in carbon website or I should do that CUSTOM LOGIC , as I think renaming the component might break many things in website.
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.
Ah I think I know what's missing. This needs to get added to the data/components.json for Menu Button to show up.
{
"component": "Menu button",
"parentComponent": "Menu buttons",
"overview": false,
"testing": {
"screenreader": "manual"
}
},
0bfd37b
to
040da7d
Compare
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.
Tested locally, LGTM. Not sure why Vercel is refusing to deploy 😞
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.
Looks good, thanks for fixing Riddhi! 🚀
Closes #4241
Added Combo button and Overflow menu to a11y tab in menubuttons and fixed github link for menu button
Changelog
New
Added Combo button and Overflow menu to a11y tab in menubuttons and fixed github link for menu button