Skip to content

Add focus highlight to previous/next buttons in center panel #1146

Merged
demiankatz merged 2 commits into
UniversalViewer:devfrom
Saira-A:1077
Oct 21, 2024
Merged

Add focus highlight to previous/next buttons in center panel #1146
demiankatz merged 2 commits into
UniversalViewer:devfrom
Saira-A:1077

Conversation

@Saira-A

@Saira-A Saira-A commented Oct 18, 2024

Copy link
Copy Markdown
Contributor

Fixes #1077

@vercel

vercel Bot commented Oct 18, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 9:37am

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Saira-A -- looks much better, but a couple of possible suggestions for finishing touches...


this.$nextButton = $('<div class="paging btn next" tabindex="0"></div>');
this.$nextButton = $(
`<button class="btn btn-default paging next" tabindex="0" title="${this.content.next}" style="position: relative; padding-right: 0px; padding-top: 0px; display: inline-block; top: 131.5px;">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the tabindex="0" from these buttons because button elements automatically have a tab index.

I also wonder if it would be a good idea to move the hard-coded styles out of the code here and into the appropriate LESS files.

@LanieOkorodudu

Copy link
Copy Markdown
Contributor

@demiankatz, I have tested the fix, and it works as expected. It makes sense to have the focus highlight move to the next or previous buttons when clicked within the image. Thanks @Saira-A.

@Saira-A

Saira-A commented Oct 21, 2024

Copy link
Copy Markdown
Contributor Author

Done, thanks @LanieOkorodudu @demiankatz

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Saira-A, looking great!

I think there's still a problem where the next/previous buttons can fade out of view before you have time to focus on them, but that's a separate issue that may already have its own ticket, so I don't think we need to solve that problem here -- this fixes a different, unrelated problem and might as well be merged as-is!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Community Sprint COMPLETED

Development

Successfully merging this pull request may close these issues.

3 participants