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

[BBT-105] Display custom notification when related background and foreground colours fail accessible colour contrast ratios #109

Draft
wants to merge 10 commits into
base: release/1.0.0
Choose a base branch
from

Conversation

thatisrich
Copy link
Contributor

Description

BBT-105 - Based on the colour contrast PR created here, this PR adds a custom component to provide more visual feedback to the editor around the colour contrast of the background and text colour options.

In this component, the editor can see the selected colours contrast ratio and whether the colours comply to AA and AAA standards. This notice is placed above the background and text colour palettes.

Change Log

  • src/editor/styles/index.scss - Including styles
  • src/editor/components/StylesColor.js - Adding custom component
  • src/editor/components/BBContrastChecker.js - The custom contrast checker component
  • src/editor/styles/components/bb-contrast-checker.scss - Adding styling for the custom component

Steps to test

  • Select a block or element that has the text and background colour options, such as Elements > Button
  • Select a text colour from the available palette
  • Select a contrasting colour for the background from the palette
  • Notice no warning is displayed
  • Select the same colour for the background that is assigned to text
  • Notice a contrast warning is now displayed

Screenshots/Videos

Custom colour contrast component demo

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@thatisrich thatisrich marked this pull request as draft September 25, 2024 08:36
@jonnywatersbb
Copy link
Member

I think I do prefer this method to #108 as not only do you get more context to the pass / fail but there's less movement in the UI as you are changing colours. I guess my only thought is should we display a notification when the colours pass, which would stop any movement in the UI at all? It's still slightly annoying that the UI jumps between the 3 different states:

http://bigbite.im/v/AeTBY3

There's another issue when using the colour picker and moving to another colour that changes the state. Once the notification appears / disappears it moves the cursor to a different area within the colour picker. You can see that happening at the end of the video

@g-elwell
Copy link
Member

I agree, it'd be really nice to have this.

Showing it persistently would solve a lot of problems. Can we mock up a design that fits WP branding and is maybe more streamlined, so it takes up less space if it's always going to be on screen?

I'm probably going to merge #108 anyway with the view to replacing it with this custom component when it is ready. Should be a straight component swap at that point as it will accept the same props.

@thatisrich
Copy link
Contributor Author

When researching into how WordPress handles their ColourContrast component, they seem to display the notice after the colour inputs (whether palette or other). I wonder if switching this custom component to wrap the <div> containing the colour palettes could be an option?

Approaching it this way, we could display the WCAG Check at the top as it already is, and then display the notice after the colour palettes. That approach would mean the user can see either / both the pass / fail block and the notice at all times and the layout would not be affected if the palette is in use, regardless if the debounce time has been reached or not.

This way, it could also allow additional features in future like accepting an array of colours, instead of a single colour, and compare across each variation. This would allow us to consider the <Gradient> component and colours selected within there, as that is currently missing from the core and custom components, without affecting the layout.

Let me know what you both think!

@jonnywatersbb
Copy link
Member

@g-elwell happy to merge #108 as we work on this, as this might go in a few different directions

@g-elwell
Copy link
Member

display the notice after the colour palettes

I considered this when testing #108 but worried it might end up too far away from the picker. We could try updating the colour component overall UI to address this, but I think the ideal solution to aim for would be a single custom component which replaces the need for the core one entirely, rather than having two similar notices in different locations.

This way, it could also allow additional features in future like accepting an array of colours, instead of a single colour, and compare across each variation. This would allow us to consider the <Gradient> component and colours selected within there, as that is currently missing from the core and custom components, without affecting the layout.

I like this idea, hopefully it would still be possible without wrapping the component, perhaps by passing gradient as an extra prop? I think we should keep this option open, as it's extra info which will be useful to a user.

@thatisrich
Copy link
Contributor Author

I realise this is moving into something a little more than just an inline notice, but I wonder if a more Themer UI wide accessibility notice could work here?

It's certainly valuable to display a notice alongside each instance where colours are selected, which #108 addresses, so maybe this custom accessibility checker moves to be a top level component and sits in the grid that displays the customisation options and theme.json output? This way it can be a 'your theme is AA accessibleoverthis component is not accessible`.

As a very quick browser-hacked mockup, please see a screenshot showing when the theme.json output is stacked in the layout, and when it sits alongside the styles panel.

@g-elwell
Copy link
Member

I think the top-level notice is another good idea, but would first like to have the advanced contrast component shown inline when choosing colours. I think it's more valuable there in the sense it shows you the specific contrast ratio of the colours you are choosing.

A top level component would need to look a bit different, it can't give you a specific ratio or rating as it's not simply comparing two colours, so can probably only tell you if there are any likely contrast issues anywhere in your theme. This would be beneficial so you can identify these without having to dig around to find them.

I think we may need some discussions around how this behaves, what it looks like, and how it fits into other upcoming changes (like re-implementing the visual preview). I wouldn't want to derail or slow down adding this advanced checker inline, so perhaps we can open a new issue with the top level notice idea as a suggestion?

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

Successfully merging this pull request may close these issues.

3 participants