feat(Banner): add support for status icons

[thatblindgeye]

What: Closes #8113

Banner examples

An alternative approach that I ultimately didn't go with involved having the variant prop be used solely as a prop to render a status icon + screen reader text (instead of using it to determine banner color as it does now), and adding a new color prop that would instead handle the banner color separate from status. That would've also involved using the pf-m-red etc classes instead of pf-m-danger etc ones. That sort of approach could possibly work better for the breaking change/v5 release.

Additional issues:

[patternfly-build]

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

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

[mcarrano]

Looks good. Thanks @thatblindgeye !

[mcoker]

nice! left some comments, lemme know what you think!

[mcoker]

I should have made a note about this, but we currently don't manage the layout of the banner contents at all. All of our status examples should really be demos, since we're adding a flex layout to space the text and icon. In practice, it's up to the user to manage the layout and spacing and everything. For example, they may want to have a banner like this:

-----------------------------------------------------------
| Some text          [icon] Alert text          More text |
-----------------------------------------------------------

You can see some examples in the design docs - https://www.patternfly.org/v4/components/banner/design-guidelines#when-to-use

So they'll need to be able to choose the layout and spacing and everything. I think if we have some examples, that should be good. cc @mcarrano does that all sound right?

[thatblindgeye]

That's a good point. In that case, we may not even want to have a prop that determines whether an icon gets rendered, but rather just show an example of banners to convey status? In that case the example would just have the icon and banner text passed in as children of Banner.

[mcoker]

@thatblindgeye yep!

[tlabaj]

So, should this not be in the demo section rather than an example.

[mcoker]

FWIW we changed the "status" variant names to color variants since the color banner used doesn't always convey status. AFAIK we plan on removing the status variants in favor of the color names in the major release.

[thatblindgeye]

That's something I was considering when working on this and ended up opting to keep this as-is for PF4. If it would make sense to update the class that gets applied to the pf-m-[color] classes I can do that, though, since we should be able to allow consumers to pass in danger etc as the variant and have that just apply pf-m-red etc.

[mcoker]

@thatblindgeye gotcha, yeah we wouldn't want to lose the prop values, or even change the class ideally, since that could be breaking. We could add the new variant names now and remove the old ones later (like we did in core) but it probably makes sense just to do it at the same time with a breaking change

Created these issues:
core - patternfly/patternfly#5162
react - #8204

[mcoker]

I think this is breaking since it works currently without hasStatusIcon.

Also is it just me or is the existing placement odd with the sr text coming after children? I think in other places, we put the sr text before the banner contents, assuming it provides context about the banner contents.

patternfly-react/packages/react-core/src/components/Banner/Banner.tsx

Lines 34 to 37 in 44af730

	>
	{children}
	<span className="pf-u-screen-reader">{screenReaderText || `${variant} banner`}</span>
	</div>

[thatblindgeye]

Re: the placement: yeah I noticed that as well and figured that was something worth updating in this PR.

Regarding the conditional rendering of the SR text: that might be iffy? With the way the SR text is currently used it's to convey the banner variant, though custom text can be passed into the screenReaderText prop to convey something other than status (like simply having SR text that says "Banner:").

Since we are moving away from using status variants to determine the banner color and such, it may come down to whether it would be considered fixing a bug (a banner that doesn't convey status shouldn't have hidden text that conveys status to SR's) or if it should be kept as-is.

[mcoker]

@thatblindgeye good points! I'm mostly interested in not breaking anyone using it currently, or these examples

patternfly-react/packages/react-core/src/components/Banner/examples/BannerBasic.tsx

Lines 6 to 10 in 17c9e1c

	<Banner screenReaderText="This is a default Banner">Default banner</Banner>
	<br />
	<Banner variant="info" screenReaderText="This is an info Banner">
	Info banner
	</Banner>

If we're going to rely on color variants alone, and it's up to the dev to handle the status or whatever the banner is, it seems like we would want to always expose this prop, and/or show how to add screen reader text to the banner wherever it's appropriate to use. ie <Banner>banner text blah <span class="pf-screen-reader">sr text</span> apples and oranges</Banner>

Just an aside, but do you think it makes sense to have screen reader text as a component of some sort? Currently that's part of the PF global CSS - https://github.com/patternfly/patternfly/blob/e362c95792e234592911881cb3c818177c023fd5/src/patternfly/base/_common.scss#L25-L36

[thatblindgeye]

If we're going to rely on color variants alone, and it's up to the dev to handle the status or whatever the banner is, it seems like we would want to always expose this prop, and/or show how to add screen reader text to the banner wherever it's appropriate to use. ie banner text blah sr text apples and oranges

If it would be breaking, we could possibly open an issue as part of the breaking change or v5 release. I could revert it so that the SR text always renders, but maybe just update the examples to either provide alternative, non-statusy sounding text ("This is a blue banner...") or making it so that you can have the SR text omitted (either passing in null or an empty string?). Wdyt?

Just an aside, but do you think it makes sense to have screen reader text as a component of some sort? Currently that's part of the PF global CSS

That's an interesting idea... I might be on the fence about it? 😆 On the one hand, it could be relatively handy to just have a component you can pass in somewhere with its own props for things like full-width or absolute styling (or even as a variant of our existing Text component).

On the other hand, I guess I'm not entirely sure whether its usage or benefit would be significant, and I might be wary about it being something exposed for public use without some good a11y docs. I did come across this, though: Screen reader only component so it's definitely something others have thought of.

[mcoker]

If it would be breaking, we could possibly open an issue as part of the breaking change or v5 release. I could revert it so that the SR text always renders, but maybe just update the examples to either provide alternative, non-statusy sounding text ("This is a blue banner...") or making it so that you can have the SR text omitted (either passing in null or an empty string?). Wdyt?

Ah! I didn't notice that the sr text always renders - I thought it only rendered if you passed a value to the prop. Yeah, sounds good to me, especially since we are leaving the status variants in the component, and updating for v5.

It does beg the question of whether we should retain status variants alongside colors for uses like this, as opposed to replacing status with color. As in, have red/green/blue/etc and /danger/success/info, which would potentially allow for automatic injection of SR text (and other things we determine are useful) and potentially a separation of color global vars for blue/red/green and status global vars for info/danger/success.

[thatblindgeye]

It does beg the question of whether we should retain status variants alongside colors for uses like this, as opposed to replacing status with color.

I think the fact that anything could be passed into the Banner, including an icon, does make things a little more gray. The variants could still have some usefulness, but could arguably be replaced by a more fitting, existing prop (screenReaderText could just be passed in to render "Danger banner:" rather than needing to pass variant="danger", but having that text auto-render could be useful and "variant" might be more easily understandable without a prop description)

[thatblindgeye]

@mcoker made updates to this PR based on the convos above. @mcarrano i re-requested review from you just because there was significant update from when you approved, though the end result is technically similar.

[mcoker]

This looks fine, but in the core examples we added a <Flex> layout to handle the icon/text spacing. The whitespace character makes the space a little tight.

Is that OK to add here, or do you think it's confusing, since users might think the flex layout is needed when we're just providing an example to show the ideal spacing?

[thatblindgeye]

I think it could be fine. I could also add in some verbiage about the example using Flex components/layout to achieve the desired spacing if that would help make it clearer about what's going on in the example. Maybe something like:

"In the following example, a flex layout is used inside the banner content to show one possible way to create spacing between the icons and banner text."?

[mcarrano]

This still looks good from a UX perspective. Thanks @thatblindgeye !

[wise-king-sullyman]

🔥

[mcoker]

Nice! Looks great 👍

[mcoker]

🏆

[tlabaj]

I am not really a fan of using null here. Can we instead check the variant value at line 39. If it is default, don't show the text.

[thatblindgeye]

We could, though it would still render status/severity text for a banner that may only be using a "success" variant for the green color rather than for status for example. The null check was essentially just a way to allow selecting a variant for the color only, similar to how the Core banner examples are now setup.

Michael did open issues for replacing the status variant with color (#8204), though that's for the breaking change. If we'd want to wait for that to fix that up I could implement this suggestion of only rendering the SR text for non-default variants, otherwise we could perhaps introduce a new prop (color as mentioned in the original comment, or maybe a hasStatus to determine whether sr text gets rendered).

[tlabaj]

In that case I don't think we should default the value of the screen reader text at all, but leave it up to the consumer to set. Once we change the variant names to use the colours, they API will not make sense.

[thatblindgeye]

That makes sense. So rather than on line 39 having screenReaderText || ${variant} banner``, instead we'd just want screenReaderText, and only render that `pf-u-screen-reader` element if the prop is passed in. If that's the case I'll update the logic + examples

[tlabaj]

@thatblindgeye Yes

[tlabaj]

LGTM

[patternfly-build]

Your changes have been released in:

• @patternfly/react-catalog-view-extension@4.92.50
• @patternfly/react-code-editor@4.82.50
• @patternfly/react-console@4.92.50
• @patternfly/react-core@4.257.0
• @patternfly/react-docs@5.102.57
• @patternfly/react-inline-edit-extension@4.86.51
• demo-app-ts@4.204.5
• @patternfly/react-log-viewer@4.87.45
• @patternfly/react-table@4.111.28
• @patternfly/react-topology@4.90.5
• @patternfly/react-virtualized-extension@4.88.50

Thanks for your contribution! 🎉