feat(notification-badge): added expanded state #5048
Conversation
|
Preview: https://patternfly-pr-5048.surge.sh A11y report: https://patternfly-pr-5048-a11y.surge.sh |
|
@mcarrano yep, that's probably a good idea. I can go through the core demos and make any updates needed in this PR, or we can make that a separate task. I already updated the notification drawer demos in this PR which I think is all that needs to be updated. Then we'll need a react issue to do the same. We may also consider removing the notification bell from the default masthead used in full-page demos, and only including it when clicking on it opens a notification drawer. Otherwise the basic full-page demo masthead would just include generic actions and basic functionality (a dropdown or whatever would trigger a menu, but with generic items). WDYT? |
|
That all sounds good @mcoker . Maybe open follow-up issues and I'll pull them into a future milestone? |
|
|
||
| ```hbs | ||
| <div class="pf-t-dark"> | ||
| {{#> button button--modifier="pf-m-plain" button--attribute='aria-label="Notifications" aria-expanded="true"'}} |
There was a problem hiding this comment.
Were you using aria-expanded="true" here instead of button--IsExpanded="true" to avoid applying pf-m-expanded to the button itself?
There was a problem hiding this comment.
Yeah, it's only supported on the control button, and noticed the notification badge button in the masthead was using the expanded class, so updated that just to be aria-expanded, too. Looking at the react component, I think we'll want to add that there, too - https://github.com/patternfly/patternfly-react/blob/main/packages/react-core/src/components/NotificationBadge/NotificationBadge.tsx
| @@ -108,9 +111,15 @@ | |||
| color: var(--pf-c-notification-badge--m-attention--Color); | |||
|
|
|||
| &:hover { | |||
| --pf-c-notification-badge--after--BackgroundColor: var(--pf-c-notification-badge--m-attention--hover--after--BackgroundColor); | |||
| --pf-c-notification-badge--m-attention--after--BackgroundColor: var(--pf-c-notification-badge--m-attention--hover--after--BackgroundColor); | |||
|
Just checking - is this the intended dark theme color (especially for the attention one)? Just seems off to me. 🤷 Also, I see a slight change in the icon color on hover for the gray one but not the others; again, just checking that is intentional. |
|
🎉 This PR is included in version 4.211.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #4664