patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

feat(NotificationBadge): added expanded state

Open thatblindgeye opened this issue 3 years ago • 2 comments

What: Closes #7379, closes #6142

Notification badge examples Notification drawer demos

  • Added an isExpanded prop to the Notification Badge component
  • Updated the examples so that they match Core (showing read, unread, and attention variants)
  • Updated the examples so that the onClick handler updates the expanded state, rather than the variant
  • Updated Notification Drawer demos to use this new expanded state
  • Reordered the props alphabetically

Additional issues: N/A

thatblindgeye avatar Sep 19 '22 18:09 thatblindgeye

Preview: https://patternfly-react-pr-8010.surge.sh

A11y report: https://patternfly-react-pr-8010-a11y.surge.sh

patternfly-build avatar Sep 19 '22 18:09 patternfly-build

@thatblindgeye and @mcoker shouldn't there be a new "expanded". example, Or at least a way to switch the basic? example?

Actually I see the difference now. You built it into each example. I admit I was confused about the use case for 'isExpanded` in the in context of just the notification badge example. That could just me though. @mcarrano Maybe some descriptive text would help.

tlabaj avatar Sep 23 '22 19:09 tlabaj

@tlabaj @mcarrano it might depend whether we intend for the Notification Badge to ever be used without the Notification Drawer (maybe instead of a drawer, the badge links to a "notifications" page or something)? I could either add in something like:

The following example.... The isExpanded property is also passed in to visually indicate that the badge is expanded, for use-cases as seen in our [notification drawer](...) demos.

Or if we intend for the badge to be used independently of the drawer, I could revert the current examples a bit and add a new one for this new prop (with similar verbiage as above).

thatblindgeye avatar Sep 27 '22 16:09 thatblindgeye

@thatblindgeye I think that the text you proposed would be helpful to convey what's going on here.

It's a good question whether this might ever be used for something other than the drawer. To that extent I wonder if calling this an "expanded" state is a bit misleading. It's really more like a selected or active state that can be more generally used to reflect state. @mcoker what do you think of that? I think "expanded" came from our discussion of the need to visually reflect state of masthead items, but is this the right semantic?

mcarrano avatar Sep 27 '22 18:09 mcarrano

@mcarrano good question... I don't see why it would necessarily need to be tied to the notification drawer. It's basically just a plain variant button with unique background/text color. It could be used as tabs or something, too, or maybe some sort of filter.

It's worth noting that besides the naming being more flexible between selected and expanded, there is a bit of a code difference between the two, in that at a minimum, an expanded toggle would have aria-expanded="true" and a selected toggle, at least in some circumstances, would not. So it may be useful to have both - or maybe that's confusing? I'm curious @thatblindgeye's thoughts on that.

mcoker avatar Sep 27 '22 19:09 mcoker

I suppose in the case of the badge being selected in a tab-like scenario, another prop could be introduced to apply the same styling (or apply "selected" styling that is default the same), but apply possibly different aria attributes. That would I assume require a Core PR first to add styling for a selected (or whichever, there may be a better name for that sort of badge) modifier.

I think expanded works for this particular use-case of a badge causing a drawer to open or close. If we anticipate (or if we just want to offer the option for) a notification badge being used outside of a drawer, I can update this PR so that:

  1. The current examples are somewhat reverted, basically just removing the isExpanded prop
  2. A new example gets added, describing when to use the isExpanded prop and linking to notification drawer demos

I could see the notification badge being used in non-drawer scenarios, since any props passed into it are going to be applied to the Button that is used under the hood. So a consumer could pass in href and component="a" props to turn the notification badge into a link. I guess it just depends if we anticipate that being a significant situation?

thatblindgeye avatar Sep 27 '22 20:09 thatblindgeye

OK. Let's stick with what we've got for now and just add the text to clarify what the example is demonstrating. Sound good?

mcarrano avatar Sep 27 '22 21:09 mcarrano

@tlabaj @mcarrano Updated the example descriptions to mention the isExpanded prop. Also updated the prop description to mention the expanded styling as well as it setting the aria-expanded attribute.

thatblindgeye avatar Sep 28 '22 12:09 thatblindgeye

Would it be more correct to say "indicate the drawer is expanded?"

@mcarrano It's definitely a little trickier here since the badge does get the aria-expanded attribute set based on this prop, but it also isn't technically the badge that is expanding.

Maybe, "...set the aria-expanded attribute on the notification badge and to indicate that a notification drawer is expanded"?

thatblindgeye avatar Sep 28 '22 16:09 thatblindgeye

How about, "...set the aria-expanded attribute and apply visual styling on the notification badge and to indicate that a notification drawer is expanded"?

mcarrano avatar Sep 28 '22 17:09 mcarrano

@mcarrano Updated the description! Thanks for that last suggestion

thatblindgeye avatar Sep 28 '22 18:09 thatblindgeye