Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docs for new banner #748

Merged
merged 19 commits into from
Jun 6, 2024
Merged

Add docs for new banner #748

merged 19 commits into from
Jun 6, 2024

Conversation

lukasoppermann
Copy link
Contributor

@lukasoppermann lukasoppermann commented Mar 22, 2024

Only merge when the react component is implemented

content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
Keep the message concise and direct. Don't use headings or multiple type sizes in banners, rely on the default paragraph size and use the title when appropiate.

Don't show more than one banner at a time. For more information, see this article on [alert fatigue](https://www.nngroup.com/videos/alert-fatigue-user-interfaces/).

## Accessibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that none of the proposed Banner updates affect our existing a11y guidance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'd love to get this info form @ichelsea

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this to my list of things to look at this week!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this entire a11y section was written up form the work that @lindseywild and @khiga8 did, so it's valid! There are a couple of things that we want to adjust based on feedback from engineering office hours earlier this week that has also been on my to-do list to get to.

We can create a separate PR for that if you want to get this one merged, or I can add to it before this one is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add it to this PR. We will keep it open until the react components are implemented and shipped, so it will be open for a bit.

content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Show resolved Hide resolved
Copy link
Contributor

@langermank langermank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start!

content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
/>
<Box as="p" mt="0">
<h4>Title</h4>
Use a title to consisly present the topic of the banner when the message of the banner is complex and too long to scan.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add something about title being required but may be visually hidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshblack is this how we implement it, or is there a separate a11y title which will just use the title the user defines if they do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukasoppermann I think this is how it's implemented now where the title is required but may be visually hidden 👍 At the moment, this would mean they wrap the title in VisuallyHidden but we could also simplify this with a dedicated prop if that would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is the right place for a discussion, but I am a bit afraid that if the title is not hidden by default, we will get a lot of duplicates with title and body being the same text.

My only thought to improve this would be to have two props like aria-title and title where aria-title is set to title if present. So, using typescript we could require on of the two to be set.

But I am no react dev and this may be an uncommon or anti-pattern.

content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just left a comment/question.

content/components/banner.mdx Outdated Show resolved Hide resolved
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
Co-authored-by: Josh Black <joshblack@github.com>
@mperrotti
Copy link
Contributor

Looks good to me. I'd "approve" but waiting until this is out of "draft".

Just ping me when you're officially ready for a review 🙂

Copy link
Contributor

@ichelsea ichelsea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this documentation going, @lukasoppermann!

I have a couple fundamental thoughts around our suggestions vs our inline messaging that's coming. Should we write like we don't yet have that component or write like we will have that component soon?

content/components/banner.mdx Outdated Show resolved Hide resolved

#### Critical

Use critical banners to signal critical errors, system failures, or necessary destructive actions. Examples: form validation error summary, repo deletion confirmations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I'm following the repo deletion confirmation example. Right now if you delete a repository, you get a blue banner at the top confirming deletion. I wouldn't use the critical banner because even though it was destructive, it was a successful action. We should only be using them to bring attention to failed actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not talking about the once it is deleted, but rather before you delete.

However, I think this was somewhat recently removed. Is there another good example?

content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
content/components/banner.mdx Outdated Show resolved Hide resolved
Keep the message concise and direct. Don't use headings or multiple type sizes in banners, rely on the default paragraph size and use the title when appropiate.

Don't show more than one banner at a time. For more information, see this article on [alert fatigue](https://www.nngroup.com/videos/alert-fatigue-user-interfaces/).

## Accessibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this entire a11y section was written up form the work that @lindseywild and @khiga8 did, so it's valid! There are a couple of things that we want to adjust based on feedback from engineering office hours earlier this week that has also been on my to-do list to get to.

We can create a separate PR for that if you want to get this one merged, or I can add to it before this one is merged.

Co-authored-by: Chelsea Adelman <40274682+ichelsea@users.noreply.github.com>
@lukasoppermann lukasoppermann merged commit 8659841 into main Jun 6, 2024
6 checks passed
@lukasoppermann lukasoppermann deleted the banner-docs branch June 6, 2024 18:03
@lukasoppermann
Copy link
Contributor Author

@ichelsea I merged now as the banner already shipped, please add your improvements via a separate PR. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants