feat(dropdown): add support for menu item icons - #2451
Conversation
Sorry, something went wrong.
|
PatternFly-Next preview: https://patternfly-next-pr-2451.surge.sh |
Sorry, something went wrong.
mattnolting
left a comment
There was a problem hiding this comment.
Looks good, left a couple comments.
Sorry, something went wrong.
| @@ -63,6 +63,14 @@ import './Dropdown.css' | |||
| {{/dropdown}} | |||
| ``` | |||
|
|
|||
| ```hbs title=Menu-item-icons | |||
| {{#> dropdown id="dropdown-align-top-expanded" dropdown--IsActionMenu="true" dropdown--IsExpanded="true" dropdown--HasItemIcons="true" dropdown--HasToggleIcon="true"}} | |||
There was a problem hiding this comment.
needs a different id to pass axe testing
Sorry, something went wrong.
There was a problem hiding this comment.
updated
Sorry, something went wrong.
| {{#if dropdown--HasItemIcons}} | ||
| {{#> dropdown-menu-item dropdown-menu-item--type="a" dropdown-menu-item--modifier="pf-m-icon" dropdown-menu-item--attribute='href="#"'}} | ||
| {{#> dropdown-menu-item-icon}} | ||
| <img src="/assets/images/pf-logo-small.svg" alt=""> |
There was a problem hiding this comment.
needs aria-hidden="true" val or alt text
Sorry, something went wrong.
There was a problem hiding this comment.
from #1916 (comment)
Since you are using <img> elements for these icons, AND since these images are redundant (i.e. the text that follows is considered synonymous with these icons), then having alt="" like you have it here is the correct way to tell assistive technology that this image provides no extra meaning and to ignore it.
Sorry, something went wrong.
There was a problem hiding this comment.
Still, doesn't pass axe testing. If @jgiardino ok'd it in the past, I'm good with it.
Sorry, something went wrong.
mattnolting
left a comment
There was a problem hiding this comment.
LGTM 👍
Sorry, something went wrong.
christiemolloy
left a comment
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
|
🎉 This PR is included in version 2.43.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sorry, something went wrong.
fixes #2450
I think changing pf-c-dropdown__menu-item to display: flex would be a breaking change, so I've created a modifier (.pf-m-icon) to add that, then added a new element to support the icon configured the same as the app launcher.