Improve accessibility of sidebar toggle on mobile element index pages, darken sidebar headings#11169
Improve accessibility of sidebar toggle on mobile element index pages, darken sidebar headings#11169brandonkelly merged 17 commits into4.1from
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 |
| }, | ||
|
|
||
| toggleSidebar: function () { | ||
| toggleSidebar: function ({target}) { |
There was a problem hiding this comment.
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.
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.
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.
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
2 participants
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