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

Proposal: Improve Notification colour contrast (a11y) #446

Closed
m10l opened this issue Mar 30, 2020 · 11 comments
Closed

Proposal: Improve Notification colour contrast (a11y) #446

m10l opened this issue Mar 30, 2020 · 11 comments
Assignees
Labels

Comments

@m10l
Copy link
Contributor

m10l commented Mar 30, 2020

The problem

The Notification component currently fail WCAG colour contrast accessibility guidelines:

Notification with success intent:

Element has insufficient color contrast of 2.99 (foreground color: #ffffff, background color: #0baa75, font size: 10.5pt (14px), font weight: bold). Expected contrast ratio of 4.5:1

Notification with warning intent:

Element has insufficient color contrast of 2.85 (foreground color: #ffffff, background color: #db8500, font size: 10.5pt (14px), font weight: bold). Expected contrast ratio of 4.5:1

The proposed solution

I'd like to propose some changes to these components to make sure we can consistently meet accessibility guidelines by not setting text on a solid-colour background.

Here are some mockups of what I'd like to implement:

Success
Screenshot 2020-03-30 at 15 03 25

Warning
Screenshot 2020-03-30 at 15 01 35

Error
Screenshot 2020-03-30 at 15 01 17

@m10l m10l added the proposal label Mar 30, 2020
@m10l m10l self-assigned this Mar 30, 2020
@gui-santos
Copy link
Contributor

IMO, they look great! It really improves the readability of the notifications 👍

@domarku
Copy link
Contributor

domarku commented Mar 30, 2020

Great proposal @m10l ✨ I've got couple of comments:

  • This definitely makes them more readable!
  • There should be some more vertical spacing between the text elements, spacing-2xs (4px) or even xs (8px), to make the message easier to scan and the perceived click area around the CTA larger
  • The icon should be horizontally aligned with the Title / Close button. People scan messages in the form of the letter F, but this way the eye pops back left to view the icon after reading the Title. Examples from other systems: Atlassian, Polaris, Carbon
  • Can we use the TextLink component for the CTA here? It would make it stand out a bit more and consistent with the rest of the web app.
  • Consider text wrapping in the body. The right side of the notification — close button with white background — should have similar spacing as the left side — icon with coloured background, so no text goes below the close button.

Questions

  • Where is the CTA when there's no body text? An example might be a "success" message with an Undo
  • Could we rename the component to Message or Toast or would that be a breaking change?
  • Should we replace the icon in the red error message with the Error Icon to make it different than the icon for the warning message?

Here's a sketch of all the details I mentioned above:
Group

@domarku
Copy link
Contributor

domarku commented Mar 30, 2020

I see that Github has the icon vertically centered in their Toast component. Look at others to get inspired not to prove a point! :) But, nevertheless, I still think it should be aligned to the top.

@m10l
Copy link
Contributor Author

m10l commented Mar 30, 2020

@domarku Awesome, thanks for the review and suggestions

There should be some more vertical spacing between the text elements, spacing-2xs (4px) or even xs (8px), to make the message easier to scan and the perceived click area around the CTA larger

Cool, I think this is worth tackling at the same time too so will do this as part of the same PR

The icon should be horizontally aligned with the Title / Close button. People scan messages in the form of the letter F, but this way the eye pops back left to view the icon after reading the Title. Examples from other systems: Atlassian, Polaris, Carbon

The examples linked all have a solid background for the entire notification, so I think your suggestion makes sense there, but doesn't work for what we're doing as it's a smaller section with a solid colour background.

Can we use the TextLink component for the CTA here? It would make it stand out a bit more and consistent with the rest of the web app.

Ah good spot, was using TextLink but the wrong colour.

Consider text wrapping in the body. The right side of the notification — close button with white background — should have similar spacing as the left side — icon with coloured background, so no text goes below the close button.

Cool, think text is already prevented from going beneath the close button, but I'll test it to make sure.

Where is the CTA when there's no body text? An example might be a "success" message with an Undo

It's always at the bottom

Could we rename the component to Message or Toast or would that be a breaking change?

That'd be a breaking change, wouldn't recommend it for now

Should we replace the icon in the red error message with the Error Icon to make it different than the icon for the warning message?

Ooo, definitely, good shout.

@domarku
Copy link
Contributor

domarku commented Mar 30, 2020

Right on @m10l! Thanks for addressing all my comments. Any chance you would consider putting the CTA inline in case of no body text? Like so:
Group Copy 8

@m10l
Copy link
Contributor Author

m10l commented Mar 31, 2020

Right on @m10l! Thanks for addressing all my comments. Any chance you would consider putting the CTA inline in case of no body text? Like so:
Group Copy 8

@domarku Still not in favour of doing this, I think introduces more potential problems than it tries to solve.

Pros/cons

  • ➕ Everything readable on a single line
  • ➖Concerns around truncation - how to handle if the CTA and title are both super long? What if the title spans multiple lines?
  • ➖Consistency - I'd argue that moving the CTA based on number of lines of text could create a jarring experience for the user

@domarku
Copy link
Contributor

domarku commented Mar 31, 2020

Ah, good points. Although that makes me think we should have come copy guidelines for the notifications. I'll give it a go at writing and adding them to the thread later in the day.

@m10l
Copy link
Contributor Author

m10l commented Mar 31, 2020

@domarku That'd be awesome! Shall I close this an open a PR?

@domarku
Copy link
Contributor

domarku commented Mar 31, 2020

Do it 🤠

@wichniowski
Copy link
Contributor

looks really good! now if you compare the old notification to it, the old one look super outdated and wrong so 100x for the proposal

@m10l
Copy link
Contributor Author

m10l commented Apr 2, 2020

Closing as PR created here: #449

@m10l m10l closed this as completed Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants