Skip to content
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

Lightbox Keyboard Support #1667

Merged
merged 12 commits into from
Sep 22, 2020
Merged

Lightbox Keyboard Support #1667

merged 12 commits into from
Sep 22, 2020

Conversation

crhallberg
Copy link
Contributor

  • Constrain keyboard focus to lightbox.
  • Add ESC to close lightbox.

@crhallberg crhallberg changed the base branch from master to release-5.1 July 7, 2020 19:45
@ckaz
Copy link
Contributor

ckaz commented Jul 7, 2020

You are amazing! Will look at this tomorrow. Good night!

@ckaz
Copy link
Contributor

ckaz commented Jul 8, 2020

Just tested this -- works like a charm! Thanks!
Just wondered whether it would be more accessible if we hid the "icon" in the close button like so:
<button type="button" class="close" data-dismiss="modal" aria-label="<?=$this->transEsc('Close modal') ?>"><i class="fa fa-times" aria-hidden="true"></i></button>

@demiankatz
Copy link
Member

@crhallberg / @ckaz: this PR has some conflicts that need to be resolved. I'm curious if it's still worth the effort of backporting this to release-5.1 and then merging it all the way forward, or if we can just move development to the bleeding edge. If having it in release-5.1 would help @ckaz, I'm certainly willing to continue that approach, but since 6.1.2 has now been released, I'm not expecting any further 6.x releases unless a major bug is discovered, so it may be better to refocus on new development. Thoughts/opinions?

@crhallberg
Copy link
Contributor Author

I'd be happy to resolve as far back as would be useful for @ckaz.

@ckaz
Copy link
Contributor

ckaz commented Aug 10, 2020

Sorry for taking so long. I was on vacation. Please don't put too much effort into backporting this. I could easily create a working version in our standard theme and take it from there!

@demiankatz
Copy link
Member

Thanks, @ckaz; @crhallberg, can you re-target this against the dev branch and resolve any outstanding conflicts?

@demiankatz demiankatz changed the base branch from release-5.1 to dev September 8, 2020 12:30
@demiankatz
Copy link
Member

@crhallberg, thanks for the progress on this. I noticed that it was still based against the release-5.1 branch; I've changed it to use dev as the base branch, which required me to recompile the LESS to resolve conflicts. Do you mind giving this one more review to be sure I haven't messed anything up?

EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Sep 15, 2020
@demiankatz
Copy link
Member

@crhallberg, I notice that there seem to be some Javascript style issues that are preventing the build from passing here... Can you fix?

@demiankatz
Copy link
Member

All tests are passing; merging now! Thanks, everyone.

@demiankatz demiankatz merged commit b4d78db into dev Sep 22, 2020
@demiankatz demiankatz deleted the accessibility-js branch January 21, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants