feat(AlertGroup): Introduce Alert Group component - #1136

Deploy preview for pf-next ready!

Built with commit d15739c

https://deploy-preview-1136--pf-next.netlify.com

Should toast groups be vertically scrollable?

My 2 cents, from looking at the designs, I don't think we would want a scrollbar. Looks like the list will only show 3 alerts at a time, and if there are more than 3, the 3rd one says to use the notification drawer. So I don't think there will ever be more than 3 there. I suppose that is the UI to use for both toast and inline alert lists? This is also probably a question for @mcarrano and/or kyle/michael.

I like the use of padding here to keep the list centered, but I think you can write this in a shorter way and also with the padding, the list isn't actually 600px wide. It's 600px - the left/right padding.

What do you think about this instead...

    top: var(--pf-c-alert-group--m-toast--PaddingTop);
    right: var(--pf-c-alert-group--m-toast--PaddingRight);
    pointer-events: none;
    width: calc(100% - calc(var(--pf-c-alert-group--m-toast--PaddingRight) * 2));
    max-width: var(--pf-c-alert-group--m-toast--MaxWidth);

Except replace the PaddingTop/Right vars with Top/Right

@andybraren @mcoker There definitely should not be a scrollbar. Theoretically you could stack as may notifications as you want, but we would never advise more than 3.

I'm curious about the thinking behind making this a component. Is this intended as a general container that I put a toast alert in? So if I only have a single alert, do I start with a group of one and then add other alerts to it?

I'm curious about the thinking behind making this a component. Is this intended as a general container that I put a toast alert in? So if I only have a single alert, do I start with a group of one and then add other alerts to it?

@mcarrano good question. As I understand it, the way it is now, this would be the container for all toast alerts, to position them in the top/right, whether you have one or 30. However a single inline alert can still be displayed on the page inside the alert group component or just on its own outside of the alert group component, and the alert will look the same visually. But if you have more than 1 inline alert, you would want them to be in the alert group to get the vertical spacing. Does that sound ok?

Thanks for the clarification @mcoker . I think that all makes sense. It might be good to document that somewhere in the description so consumers understand what this is and how to use it to position toast notifications. There is an issue in the backlog for an Alert and Notification demo that should also help clarify this. #625

I guess the scrollbar question could be re-phrased: Should AlertGroup provide an ugly-yet-functional safety net (scroll functionality) to assist users if/when developers fail to implement their notification system exactly as we've designed?

I can see why we lean toward no, but I also wonder what % of devs will actually implement the collapsing "Max 3" design in the real world given the seemingly more complex JS logic involved. If we can provide solid React/JS code that works out of the box like everything else that's great, but I don't know enough about that world to scope out how difficult that will be.

@andybraren good question about the scrollbar. I think my preference would be that we don't include a scrollbar and if there are more than 3 alerts, they will just continue to stack and grow the height of the container. This gives a project using this an option of showing all notifications, even though I wouldn't recommend that, or setting some other criteria for showing or hiding the notifications. Thoughts?

@mcarrano I'm okay with that, and this component supports that behavior as-is, it'll just look like this (unscrollable, bottom-most alert cut off until a higher-up alert is dismissed):

After discussing this PR with @gdoyle1 and @mcarrano there are two more updates to make:

1. The Toast example should include a unique "collapsed" third toast as shown below, which is the behavior we're recommending that developers implement. Instead of "Dismiss All" the shortcut/action should be "View all" (lowercase a) and it should open the notification drawer when clicked. This toast should also include a close button.

2. The developer documentation should be updated to say that every toast needs to include a close button and should never include an action/shortcut except for the unique "collapsed" toast described above.

Updated the visuals here to match the implementation as well @andybraren !

I've updated this PR to address the above, but Toast Alerts won't quite match the visual design until the following issues are addressed:

1. The visual design for the third toast uses an "Other" alert type with a non-standard grey color. This wasn't implemented in the Alert component, so I'm using a blue Info type instead. Are we okay with this, or should we go back and add this new alert type to the Alert component? @mcarrano @gdoyle1

2. The spacing between the close button and secondary action button ("View all" in this case) doesn't match the visual design right now. The Alert component needs to be updated to fix that spacing. I've created a new issue Alert: Secondary action button and spacing #1172.

3. We might want to consider re-structuring the .pf-c-alert__action element to align with other components as described in Alert Component Action Element #774. Right now we squeeze the close button, primary action button, and secondary action button ("View all") within this one div, which seems to unnecessarily constrain the .pf-c-alert__body and its longer text (somewhat shown above, just imagine if "View all" were longer in another language).

4. Reiterating Require variant of Alert that includes Close button #1099, the Alert component should have an example of an alert with a close button using the appropriate aria-label. @jgiardino the Button component's example use aria-label="Remove", but in the context of inline alerts and toasts do you think "Dismiss" would be a better word?

@andybraren I don't really have a strong opinion on using the blue info type instead of the grey one designed - the intent of designating that alert with a distinct color was because it only applies to a single use case. Although, I do think that use case would generally fall under the category of "info" alerts. @mcarrano Do you think this falls under "general informational messages"?

@andybraren @gdoyle1 a couple of comments...

I think that we should stick with the blue info color for this alert as we are not using that grey color anywhere else for an alert. @kybaker , any problem with that? Not sure why gray was used in the original visual.

In this case, I'm questioning whether both 'View all' and 'X' should both be there. I know we said the user sure always be able to dismiss a toast notification, but I see this as a special case. The third toast should disappear on it's own when there are 3 or less actual notifications either because the user cleared them or because they expired. Does that make sense?

@mcarrano I get your point with removing the 'x' on the last toast, but would only say that if it does not have an 'x' then it wouldn't follow the documentation we have on toasts. I don't really see a harm in leaving the 'x' there in case the user wants to immediately dismiss that toast before the other two on top of it. Otherwise, if they were to just 'x' out one on top (or one were to disappear on top), that last toast would disappear automatically (I think that was what you were saying).

@mcarrano @gdoyle1 As an impatient user I wouldn't enjoy having to either wait 8 seconds or click more than 3 X's to make all toasts go away. I'd vote for always having an X, even on the special third toast.

And to clarify the behavior of that special third toast, looking at this image for example:

Are we saying that dismissing/closing the second red toast would make the third blue toast dissolve and reveal the remaining two (older) notifications? I wonder if triaging 4+ notifications two at a time like that will feel better than keeping the third toast static as just a heads-up that there are # more alerts in the notification drawer. I also wonder what the animations should be if/when the special toast goes poof.

@andybraren and @gdoyle1 these are all good points. I just finished a conversation with @matthewcarleton about all of this and we agreed that it may be time to rethink the design of this a bit. There is clearly more complexity to the toast aggregation than initially considered. At the same time we want to avoid adding more complexity to the Alert component itself.

I am going to set up a meeting so we can talk through this live and consider alternatives for keeping this moving forward.

Reiterating #1099, the Alert component should have an example of an alert with a close button using the appropriate aria-label. @jgiardino the Button component's example use aria-label="Remove", but in the context of inline alerts and toasts do you think "Dismiss" would be a better word?

"Dismiss" sounds like the right action word for the Alerts in a group.

Also, in similar patterns like ChipGroup and DataList where items in a list have the same action, we're also labeling the action with the main text of that item. This is helpful for providing context for buttons that otherwise would have identical labels.

For example, the remove button in a chip has the following attributes:

  <button class="pf-c-button pf-m-plain" aria-labelledby="remove_chip_one chip_one" aria-label="Remove" id="remove_chip_one">
    <i class="fas fa-times-circle" aria-hidden="true"></i>
  </button>

If the Chip label is "Puppies", the button is announced as:

Remove Puppies button

In this case, the dismiss button should reference the heading element of the Alert:

    <h4 class="pf-c-alert__title" id="alert_one_title">
      <span class="pf-u-sr-only">
       				Success: 
     </span> Files are uploaded
    </h4>

And the combination of the Dismiss label and the heading text would be announced as:

Dismiss Success: Files are uploaded button

But I'd love some suggestions on refinements to this. Like should we include the word "alert" in the hidden text (i.e. "Success alert:"), should we drop the heading text and only reference the type of alert (e.g. "Dismiss Success alert: button") and rely more on context for clarification to avoid potentially really long buttons labels? Given the number of alerts that could display and the likeliness that they would be removed automatically, I don't imagine there being a large number of any one type of alert anyway.

I'm cc'ing @stacymcauliffe in case she has input, too.

@jgiardino - So, in order, the user would hear something like -

"Success: Files are uploaded" (plus maybe something that indicates to them that this is an alert)

and then when they navigate to the next element,

"Dismiss Success Files are uploaded button"

or

"Dismiss success alert button"...

I think it would be enough to read "Dismiss success alert button" but it's hard to say definitively without understanding how easy it is for context to be lost/forgotten. I would imagine they'd be hearing the information contained in the alert message and then immediately either clicking a CTA/action button in the alert or simply dismissing.

@stacymcauliffe

Yes. There's also the case where they're only scrolling through all the buttons on the page. So they could hear:

"Dismiss Success alert button"
"Dismiss Success alert button"
"Dismiss Warning alert button"

If they're navigating by button, they could shift focus to the previous/next element next to that button to get a sense of context. But they would need to somehow know when they're on a button whether that alert text that comes before it or after it is the alert they're dismissing, which could be challenging.

I'm still really on the fence with this, but for that reason I slightly lean toward the more verbose, obvious method of labeling.

When you say

(plus maybe something that indicates to them that this is an alert)
That isn't there currently, but that would be good. I noted this in the description for #854 but included a question for you in that issue.

Thanks for making these updates! There were a couple of things I noticed:

• Are we referring to these as "Notifications" or "Alerts"? Maybe that's something that was already discussed, but just wanted to double-check. When chatting with @stacymcauliffe, we settled on using this label for both dynamic and static alerts. I'm not sure if one works better for both, so I'll defer to others on this on.
• There are a couple of tweaks needed for the attributes on the Dismiss button.
  • This is what it is currently, which would be announced as "Success notification: File was uploaded button" (assuming the heading is "File was uploaded"):
    <button class="pf-c-button pf-m-plain" aria-label="Dismiss" aria-labelledby="notification_three_title">
  • This is what it should be, which would be announced as "Dismiss Success notification: File was uploaded button":
    <button class="pf-c-button pf-m-plain" aria-label="Dismiss" aria-labelledby="notification_three_button notification_three_title" id="notification_three_button>
    • The aria-labelledby attribute will override the aria-label value, and the only way to announce the aria-label text string that is by referencing the button id in the aria-labelledby value

I’ve updated this PR to:

1. Rebase
2. Switch to .pf-c-screen-reader per @mcoker’s review
3. Fix the accessibility issues that Jenn noted (I didn’t know aria-labelledby could be concatenated like that, cool, thank you Jenn!)
4. Add role="alert" to toast alerts with updated accessibility docs.
5. Remove redundant accessibility documentation now that the Alert component includes examples with close buttons (as of PR fix:(alert): updates alert #1208)

I’ve also created a new issue #1226 to propose some small updates we should consider making to the Alert component.

Your updates look good! And thanks for opening #1126!

I'm not able to identify what changes were made as part of

5. Remove redundant accessibility documentation now that the Alert component includes examples with close buttons

Is there a specific commit where this change was made? I'd like to see what the documentation is. Currently I don't see anything about the Dismiss button specifically. Maybe it's there and I can't find it, or maybe it's something we can note in #1126.

@jgiardino Sorry, I mis-wrote, it turns out the "redundant accessibility documentation" was about the screen reader class/utility and not related to the close buttons (whose aria-label isn't documented in that component, btw). Here is the old accessibility documentation for the Toast Alert from a previous commit:

| .pf-u-sr-only | .pf-c-alert__title <span> | Content that is visually hidden but accessible to assistive technologies. This should state the type of notification and refer to itself as a notification rather than an alert. ** Required**|

This was basically the same as the Alert component's accessibility documentation, but also noted that it should call itself a "notification" rather than an alert. We've agreed to not do that, making the above redundant with what's in the Alert component today, so I removed it and replaced it with:

| role="alert" | .pf-c-alert | Used to communicate the toast alert's time-sensitive information to screen reader users. |

Although based on the discussion happening in #1226 the location of role will need to change as well (and the above wasn't the right location to begin with).

And you're right that I haven't documented the aria-label for close buttons within a toast alert in this PR yet, and I should. Right now it's "Dismiss", but maybe it should be "Close" if we follow the reasoning around alert vs notification that Stacy presented here.

Edit: I've fixed the issues above for the sake of keeping AlertGroup and Alert in sync. Close buttons now have aria-label="Close" and the accessibility documentation of toast alerts is now:

| role="alert" | .pf-c-alert__body | Communicates contents of the alert message to the user. Required |

🎉 This PR is included in version 1.0.124 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀