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

Improve accessibility of sidebar toggle on mobile element index pages, darken sidebar headings #11169

Merged
merged 17 commits into from
May 11, 2022

Conversation

gcamacho079
Copy link
Contributor

Description

Darkens sidebar headings to meet minimum contrast guidelines. Updates toggle element to use disclosure attributes and convey purpose more accurately.

Adds a second-level heading about the element view to aid in screen reader usability.

Related issues

Resolves DEV-417
Resolves DEV-368

@gcamacho079 gcamacho079 added the accessibility 👤 features related to accessibility label May 9, 2022
@gcamacho079 gcamacho079 requested a review from brandonkelly May 9, 2022 19:03
@gcamacho079 gcamacho079 requested review from benjamindavid and a team as code owners May 9, 2022 19:03
@linear
Copy link

linear bot commented May 9, 2022

DEV-417 Sidebar toggle button has incorrect name

The button text is updated to reflect the currently chosen source. For example, if I'm looking at the "Blog" source in my assets directory, the button text is "Blog" and is updated to whatever source I choose.

Screen Shot 2022-03-31 at 12.11.37 PM.png

The button text should be modified to read "Show sources sidebar" for clarity about its purpose

Resolves CMS-093

DEV-368 Sources sidebar headings do not meet minimum contrast guidelines

Element has insufficient color contrast of 4.46 (foreground color: #606d7b, background color: #e4edf6, font size: 8.3pt (11px), font weight: bold). Expected contrast ratio of 4.5:1

To solve, darken heading color to around #5E6C78

Resolves CMS-059

src/web/assets/cp/src/js/BaseElementIndex.js Show resolved Hide resolved
@@ -524,7 +522,10 @@ Craft.CP = Garnish.Base.extend(
);
},

toggleSidebar: function () {
toggleSidebar: function ({target}) {
Copy link
Member

Choose a reason for hiding this comment

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

If we need to accept a target here, it should just be a regular target argument, rather than an object. Then update the event handler to extract the target from the event data.

this.addListener($('#sidebar-toggle'), 'click', ({target}) => {
  this.toggleSidebar(target);
});

Also, the function should continue to work if target isn’t passed in, for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the target variable, I thought I'd use a destructuring assignment to pull it out of the event object, to cut down on code inside the toggleSidebar function. I can re-work that though 👍🏼

Out of curiosity, what browsers/versions don't support target in this context? I'm not really versed on issues that would arise there. Appreciate the feedback!

Copy link
Member

Choose a reason for hiding this comment

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

Not a browser support issue; just an API clarity issue. toggleSidebar() should be something that can be called directly, without needing to pass an event object into.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I'll keep that in mind going forward

@gcamacho079
Copy link
Contributor Author

@brandonkelly I've made the requested updates to this PR

@brandonkelly brandonkelly changed the base branch from develop to 4.1 May 11, 2022 04:58
# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css.map
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility 👤 features related to accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants