[Bugfix:Notifications] Refactor Notification Accessibility#12343
[Bugfix:Notifications] Refactor Notification Accessibility#12343
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12343 +/- ##
=========================================
Coverage 21.67% 21.67%
Complexity 9618 9618
=========================================
Files 268 268
Lines 36158 36158
Branches 486 486
=========================================
Hits 7837 7837
Misses 27839 27839
Partials 482 482
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Rkoester47
left a comment
There was a problem hiding this comment.
I tested the changes made on this branch, and can confirm that the home page notification functionality remains. I was able to view, and clear my notifications as usual. The extra dead space around the mark as seen button is not an issue, in my opinion. As all checks are now passing, I think these changes successfully accomplish the goals outlined by this PR.
williamschen23
left a comment
There was a problem hiding this comment.
The heading h3 (with computed level 3) follows the heading h1 (with computed level 1), skipping 1 heading level.
Not really notification related but is still on the home page
Click "Validate page HTML" on the bottom on the home page as instructor
I can sneak that in here. |
|
@JManion32 please resolve conflicts |
Merged the course title PR content in
Why is this Change Important & Necessary?
Fixes #12340 --- Sometimes the system cypress test fails due to some accessibility issues in Notification.vue.
The element “a” must not appear as a descendant of an element with the attribute “role=link”.What is the New Behavior?
<a>to a<button>since it triggers an action, but does not follow a URL.notificationdiv into 2 containers:main-notification-content(icon, title, metadata) and the mark seen button.This refactor comes at the cost of making the small area around the mark seen button unclickable. This area can grow in height depending on how much the title wraps. In the picture below, I have marked the unclickable area with red. Please note that this is an extreme example of wrapping, in most cases it will only be a couple pixels.
Although I was initially opposed to having any dead space on the notification component, I think this small amount is actually beneficial. If users accidentally click around the mark seen, they will no longer be corralled to another page.
What steps should a reviewer take to reproduce or test the bug or new feature?
Ensure
Notification.vuefunctionality is unchanged (aside from the new dead space). Ensure user experience and accessibility is good. The accessibility Cypress test should no longer fail. Click "Validate HTML" at the bottom of the page and see that there are no errors.Automated Testing & Documentation
The notification system does not have adequate E2E testing, I will be addressing this in a PR that resolves #11908
Other information
This is not a breaking change, though it is on the home page so adequate testing is essential.
The main focus is the notifications, but I also switched the
<h3>elements for the courses ranks to<h2>to make sure no heading levels are skipped.