-
Notifications
You must be signed in to change notification settings - Fork 638
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
Conversation
…sually-hide for horizontal UI
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/CP.js
Outdated
@@ -524,7 +522,10 @@ Craft.CP = Garnish.Base.extend( | |||
); | |||
}, | |||
|
|||
toggleSidebar: function () { | |||
toggleSidebar: function ({target}) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@brandonkelly I've made the requested updates to this PR |
# 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
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