Adds content to alert react examples #7695
Conversation
|
There are a few areas that need more work/input, such as the live region examples. I tried a swing at a component overview section, so I'm curious if it goes into enough detail or if we'd want more. Also not sure what's going on with the button stuff in this PR - I think have worked on the wrong branch before/may need to roll back button files on my next push here.. |
|
Preview: https://patternfly-react-pr-7695.surge.sh A11y report: https://patternfly-react-pr-7695-a11y.surge.sh |
There was a problem hiding this comment.
@edonehoo There seems to be a problem with the preview as it's hiding a bunch of content and examples. But in skimming over the code, I think the documentation you added looks great for the most part. Just a couple minor comments.
| ## Examples | ||
| ## Component overview | ||
|
|
||
| [`Alert`](/components/alert#alert) can be paired with the following components to allow for additional functionality and customization: [`AlertActionCloseButton`](/components/alert#alertactionclosebutton) and [`AlertActionLink`](/components/alert#alertactionlink). |
There was a problem hiding this comment.
Should we explain here what these additional components are for?
There was a problem hiding this comment.
I wasn't sure if we wanted to do that, or if adding the context down by the components/properties was the goal, as seen here https://patternfly-react-pr-7603.surge.sh/components/file-upload---multiple/#multiplefileuploadstatusitem. Couldn't remember what the plans were for that example, but I could go ahead and add explanation up top if that's the better route for now
There was a problem hiding this comment.
With how many sub-components that could possibly get listed on some other components, I think explaining what the additional components are for at the top of the page might end up being too verbose and/or repetitive as the "Props" section also includes that info. It might depend what the "Component overview" section should accomplish, though. If we'd want to list out sub-components that can be used alongside the Alert and what they are for, it would be worth aligning on how we can go about that without duplicating information.
A couple of potential ways to go about it that I could think of:
- "Component overview" includes only the names of sub-components and a
"For more information about these sub-components, read the [alert props](...) documentation"(rather than having links to each individual component, mainly thinking about whether it would be obvious what exactly the first "Alert" link is linking to for users navigating via screen reader) - Include a more general description in this "Component overview" section, and go into more detail in the "Props" section
There was a problem hiding this comment.
I'm not sure either what the right answer is. I'll defer to what you think makes the most sense. It just struck me in reading this that we were mentioning these sub-components but I had no idea what they were before reading further down the page.
There was a problem hiding this comment.
I'm in the same boat about not being sure what answer is right/better. I think it may ultimately depend how the "story" should flow for the React (and later, HTML/CSS) tab and what purpose the "component overview" section should have. Taking into account how a component page may end up looking after some planned changes, below is one possibility I thought of.
Looking at the HTML tab for components, there is a "documentation" section which houses an "overview" and "usage" section, which the "usage" section seems similar to the React "props" section. Renaming that "props" section was brought up so one possibility is renaming that section to "usage" on the React tab. This "usage" section would essentially be this "component overview" section, then from there it would be a matter of should examples or "usage" come first on a component page. The structure of a component's React tab could look like:
- Component name
- Description
- Table of Contents
- Examples (or Overview)
- All of the examples
- Overview (or Examples)
- Possibly the "Composable structure" section when required, as seen in the PR Erin linked above, though it might depend if it makes more sense to keep this closer to any composable examples
- Usage; would essentially be the current props section, but with the description of the different components (if there are multiple)
- CSS Variables
There was a problem hiding this comment.
I'm a bit divided too since I'm not sure what users utilize the most on these pages. Do most people use this page to see specific examples or to quickly reference subcomponents and props? The prop tables seem to mostly just be referential information, so my inclination is that people are more interested in examples, but I'm curious if you all know what people are actually doing?
As long as referencing/linking components and props won't be too confusing for the average user, I think it may make sense for examples to come first:
- Component name
- Description
- Table of Contents
- Examples
- All of the examples
- Overview
- Possibly the "Composable structure" section when required
- Usage
- CSS Variables
I feel like the examples can be understood without first reading a props overview, especially if people just want to grab the code snippet and leave. This would also keep the component/prop context closer to the tables, so people won't have to scroll back and forth?
There was a problem hiding this comment.
@edonehoo I'd agree with what you said. I think ultimately there may be a pro/con to either way we go; either examples come first and some users are missing context of what X component or Y prop does until they continue reading further down, or users see explanations of components/props first without the context of how they all come together in examples/when they may just want the examples.
The way I might view it is the props table, component descriptions, and to an extent composable structure would be information that users can refer to when they want to use a component beyond what any of our examples show (or just for users who are interested in learning more). Whether that is the case, i.e. users are utilizing this info more than just grabbing the example code, I'm not sure.
|
|
||
| An alert can contain additional, hidden information that is made visible by clicking a chevron icon. This information can be expanded and collapsed each time the icon is clicked. | ||
|
|
||
| It is not recommended to use an expandable alert within a toast [alert group](/components/alert#alert) because the alert could timeout before users have time to interact with and view the entire alert. |
There was a problem hiding this comment.
I'm not sure if the recommendation on line 105 is clear. For me, the first part of the sentence feels like an expandable alert should never be used within a toast alert group. However, the other part (because the alert could timeout before users have time to interact with and view the entire alert) is true only if the timeout prop is used.
I'm not sure if there are any other reasons why not to use expandable alerts within toast alert group, but if there aren't any, maybe we could be more specific and change the first part to something like this:
It is not recommended to use an expandable alert with timeout within a toast...
Otherwise, we could use some better reason why not to use this combination.
@mcarrano @thatblindgeye what do you think?
There was a problem hiding this comment.
@tompsota yes, I think you raise a good point. So perhaps we can change this to something more like you suggest.
There was a problem hiding this comment.
Good catch @tompsota! I'd agree with you and Matt about changing the verbiage a bit.
Another issue with using expandable alerts within a toast alert group (or any alert with interactive content that might be necessary for a user to interact with) could be that some users would find it more difficult to interact with these alerts (see alert accessibility considerations currently on the design guidelines tab). So if important information describing the alert is in that collapsed section of the alert, a user might have to navigate through the entire page before landing on the alerts (assuming they're appended at the end of the document body).
| * Use the `actionLinks` property to add one or more `AlertActionLink` components that place links beneath the alert message. | ||
|
|
||
| * Links may also be added within the alert message using an `href`. |
There was a problem hiding this comment.
For this, we should add in something along the lines that in order to have an AlertActionLink act as a proper link (rather than a simple button), the href and component="a" properties need to be passed in.
|
|
||
| An alert can contain additional, hidden information that is made visible by clicking a caret icon. This information can be expanded and collapsed each time the icon is clicked. | ||
|
|
||
| It is not recommended to use an expandable alert using `timeout` within a toast [alert group](/components/alert#alert) because the alert could timeout before users have time to interact with and view the entire alert. See [alert accessibility considerations](https://www.patternfly.org/v4/components/alert/design-guidelines#accessibility-considerations) to understand the accessibility risks associated with using expandable alerts in toast alert groups. |
There was a problem hiding this comment.
| It is not recommended to use an expandable alert using `timeout` within a toast [alert group](/components/alert#alert) because the alert could timeout before users have time to interact with and view the entire alert. See [alert accessibility considerations](https://www.patternfly.org/v4/components/alert/design-guidelines#accessibility-considerations) to understand the accessibility risks associated with using expandable alerts in toast alert groups. | |
| It is not recommended to use an expandable alert using `timeout` within a toast [alert group](/components/alert-group) because the alert could timeout before users have time to interact with and view the entire alert. See the [toast alert considerations](/components/alert/accessibility#toast-alerts) section of the alert accessibility documentation to understand the accessibility risks associated with using toast alerts. |
Updated the links and tweaked the verbiage a bit for the end. The content currently in the design guidelines > accessibility section will be moved to the a11y tab instead.
Let me know what you think of the verbiage. My thought was that section of a11y documentation isn't strictly about expandable alerts in toast groups, but I wanted to try and convey "this section is about accessibility challenges with toast alerts, but it also applies to expandable alerts".
|
|
||
| #### Truncated alerts | ||
|
|
||
| Use the `truncateTitle` property to shorten a long alert message. Setting `truncateTitle` equal to a number (passed in as `{n}`) will reduce the number of lines of text in the alert's message. Users may hover over a truncated alert to see the full message in a popup. |
There was a problem hiding this comment.
Nitpick: maybe the verbiage could be updated to reflect that this property will only affect the alert title/the title property.
|
|
||
| Use inline alerts to display an alert inline with content. | ||
|
|
||
| #### Variants |
There was a problem hiding this comment.
This may not actually be an issue once these level 4 headings are replaced with bold text, but if both this section and the first "Variants" section are in the ToC, updating this to "Inline variants". Otherwise this should be fine as-is.
|
|
||
| It is not recommended to use an inline plain alert with actionClose nor actionLinks. | ||
| It is not recommended to use an inline plain alert with `actionClose` nor `actionLinks`. |
There was a problem hiding this comment.
Unless it could fit in either the design guidelines or a11y tabs, including why it's not recommended may be a good addition, but I wouldn't block over it.
|
Moving to a new PR to remove button commits that were unintentionally included here... |
Makes progress on patternfly/patternfly-org/issues/2990