optimize keyboard focus management on dashboard's modals - #1401
Conversation
There was a problem hiding this comment.
Since we use leanModal around the site, we should probably move this into a more common place and update all the other places where we're using similar leanModals.
Also, I'd really like to see some Jasmine tests of this if at all possible.
There was a problem hiding this comment.
Yeah I was thinking it would helpful to have like accessibility_tools.coffee somewhere
There was a problem hiding this comment.
Ideally accessibility_tools.js, but yes, I agree.
There was a problem hiding this comment.
why js and not coffee?
There was a problem hiding this comment.
We're trying to move off CoffeeScript to some degree, and all of your code is already written in Javascript already so it seems unnecessary to convert it.
There was a problem hiding this comment.
didn't realize that (that we're avoiding coffeescript)
There was a problem hiding this comment.
+1 for moving the modal behavior/interaction stuff into a central place. When we clean up the HTML/Sass, we'll be doing the same - having one modal pattern to rule them all.
|
👍 Just tested the tabbing within the Dashboard modals locally - looks good. Nice work! |
|
@adampalay cool - I've bookmarked that and will share with the Design Team as well. We're all for having a solid solution technically that we can use cross-apps and tailor UI tweaks in. Thx again! |
|
@adampalay, one more thought - the verified certs reg flow borrowed from this Dashboard modal's pattern before your fix. Can you check on that and update any necessary bits as well? |
|
@talbs , where does that live? |
|
@talbs , in a separate PR i'll look at that |
There was a problem hiding this comment.
Why immediately focus people on the close button?
There was a problem hiding this comment.
It's generally good practice. Check out the link I posted in the discussion.
|
I like it, but maybe add a |
|
cool, lgtm! |
|
closing while I write tests |
|
okay, added some tests. @dianakhuang, good to go? |
|
It looks like you might need to rebase your branch properly, but other than that, it's looking good. 👍 |
|
yeah, i don't know what's up with that. I'm also going to squash :) |
add license to a11y_tools.js add tests, some reorganization of js tests skip "toBeFocused" tests for now
|
OK, tests should be passing now, phew. @dianakhuang , one more quick look? |
|
I'm still seeing JS test failures 😟 https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/SHARD=1,TEST_SUITE=unit/947/ |
|
what? I'm skipping those tests.... hmph, investigating |
|
👍 |
optimize keyboard focus management on dashboard's modals
4 participants
@dianakhuang
@talbs
@caesar2164
Optimizes keyboard management of the modals in the dashboard view.
addresses https://edx-wiki.atlassian.net/browse/LMS-1305