-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fixed explicit label issues related to CCX accessibility #9038
Fixed explicit label issues related to CCX accessibility #9038
Conversation
Thanks for the pull request, @amir-qayyum-khan! I've created OSPR-714 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. |
308864c
to
b1a1b8f
Compare
first of two PRs @cptvitamin |
@pdpinch FYI that Mark S is out until next Monday, 3 Aug. He will likely have lots to catch up on when he returns, so I'm sure he'll get to this when he can but it may be a few weeks. |
@@ -139,7 +139,7 @@ var edx = edx || {}; | |||
return !node.hidden;}); | |||
this.$el.html(schedule_template({chapters: this.showing})); | |||
$('table.ccx-schedule .sequential,.vertical').hide(); | |||
$('table.ccx-schedule .toggle-collapse').on('click', this.toggle_collapse); | |||
$('table.ccx-schedule .unit a').on('click', this.toggle_collapse); |
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.
We prefer to target classnames rather than bare elements. Would you please provide a classname for this a
and then use that classname here in place of it? An example might be:
$('.ccx-schedule .unit .toggle-collapse')...
Notice how we're not using any specific elements.
@amir-qayyum-khan Thanks for this work. I've provided some initial feedback. Once you address those minor things I will do another pass and check accessibility. |
f08801b
to
98f7aa5
Compare
Done @clrux cc @pdpinch |
@amir-qayyum-khan Thanks! Would you mind providing instructions in this PR's description for how to preview this locally? And if you are able, a sandbox to preview this would be great. |
29898c1
to
302849c
Compare
Thanks. Done with changes that you guys suggested @cptvitamin and @clrux cc @pdpinch |
jenkins run python |
<label></label> | ||
<input placeholder="Date" class="date" type="text" name="date"/ size="11"> | ||
<input placeholder="Time" class="time" type="text" name="time"/ size="6"> | ||
<label for="date" class="sr form-label">Enter date</label> |
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.
Added additional information like formats of date and time for screen readers in https://github.com/edx/edx-platform/pull/9114
302849c
to
41be17b
Compare
@cptvitamin @clrux this PR is currently on sandbox2o.mitx.mit.edu. Ping me or @pwilkins if you need an account. |
@@ -13,7 +13,7 @@ | |||
<% _.each(chapters, function(chapter) { %> | |||
<tr class="chapter collapsed" data-location="<%= chapter.location %>" data-depth="1"> | |||
<td class="unit"> | |||
<a href="#"><i class="fa fa-caret-right toggle-collapse"></i></a> | |||
<a href="#" class="toggle-collapse"><i class="fa fa-caret-right"></i></a> |
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.
these should really be <buttons>
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.
More importantly, they have no text value. At the very least you should have screen reader text that describes the control "Toggle chapter display" with an aria-expanded attribute that describes the current state (true or false)
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.
Needs rebase. Done this in #9114
https://github.com/edx/edx-platform/pull/9114/files#diff-f15570680848e2d23dbfad465e79372dR26
With the exception of the issues on those toggle links, everything here looks great. @amir-qayyum-khan |
And a few more from me: Instructor
Coach
|
@amir-qayyum-khan can you rebase this? |
716a487
to
9d3a988
Compare
…rshp team management to fix #21
9d3a988
to
e18fa2f
Compare
Done @cptvitamin and @clrux . @clrux please created a PR https://github.com/edx/edx-platform/pull/9335 for issue cc @pdpinch |
@@ -45,7 +46,7 @@ | |||
</form> | |||
</div> | |||
|
|||
<div class="member-lists-management" style="float:left;width:50%"> | |||
<div class="member-lists-management" style="float:left;width:50%" aria-live="polite"> |
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.
@amir-qayyum-khan what kind of information in this div do you expect to change? I only ask because putting an aria-live region on such a large area can make the page unnecessarily chatty. Sometimes its necessary to limit the scope with the aria-relevant attribute.
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.
@cptvitamin I do inject error message here. I got this idea from feedback => 2nd point in section membership https://github.com/edx/edx-platform/pull/9038#issuecomment-131098884
Because this and issue below are same (identical layouts)
This is looking very nice 👍 |
@cptvitamin can you go ahead and merge this, or is more work needed? |
👍 |
Fixed explicit label issues related to CCX accessibility
Background
(copied verbatim from Mark Sadecki's CCX Accessibility audit - July 17, 2015)
"... Explicit labels are elements that programmatically associate form fields with accessible text. Many of the fields missing accessible labels in CCX have visible text labels already. They just need to be wrapped in
<label>
elements.<label>
elements can be associated with input fields in one of two ways; an implied relationship is assumed if the input is a child of the<label>
element, or, explicit relationships can be defined using afor
attribute on the element that references anid
attribute on the input."Studio Updates: None.
LMS Updates: CCX Dashboard now has explicit labels to meet edX Accessibility Guidelines.
Instructor Dashboard
Membership Tab
CCX Coach Dashboard
Schedule Tab
Enrollment Tab
Testing
You can use screen reading tools like chromeVox (A chrome browser plugin) or command+F5 on mac to verify this PR.
Also read http://edx-partner-course-staff.readthedocs.org/en/latest/getting_started/accessibility.html
Issue mitocw#21
EDX https://github.com/edx/edx-platform/pull/7855
EDX https://github.com/edx/edx-platform/pull/7921
EDX https://github.com/edx/edx-platform/pull/7873
@pdpinch @pwilkins
mitocw#26