Skip to content

Enhance focus on close buttons (fixes #1079)#1147

Merged
demiankatz merged 4 commits into
UniversalViewer:devfrom
Geoffsc:1079-FocusOnCloseButton
Oct 21, 2024
Merged

Enhance focus on close buttons (fixes #1079)#1147
demiankatz merged 4 commits into
UniversalViewer:devfrom
Geoffsc:1079-FocusOnCloseButton

Conversation

@Geoffsc

@Geoffsc Geoffsc commented Oct 18, 2024

Copy link
Copy Markdown
Contributor

Fix for #1079

@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 18, 2024 7:34pm

@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.

@Geoffsc, this looks good, but it appears that your solution relies on Tailwind classes. While Tailwind is loaded as part of the UV examples, it is not actually part of the UV itself, so I don't think this will work when the UV is used in other contexts.

I'm not totally sure what a more robust solution would be, but I wonder if it would help to define .btn.close styles in the dialogues module's LESS file.

@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 for the refactor, @Geoffsc, I think this should work more consistently across the board. Just two remaining questions:

1.) Is it just me, or does the button focus indicator seem a lighter color than everything else? I'd describe it as more of a periwinkle than a blue...

2.) Did you try using a .btn.close selector instead of .button-close so that you don't have to change the classes in the Typescript files at all? I realize that maybe you intentionally got rid of the original btn btn-default close classes to avoid inheriting unwanted styles... but I thought I'd ask just in case, because if we could do it that way, then we wouldn't have to change as many files, and there'd be less risk of breaking other things tied to the pre-existing classes (if any such things exist that we don't already know about).

@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, @Geoffsc, this looks good to me!

Not sure if @crhallberg or others might have suggestions for further optimizing/simplifying the LESS, but it clearly works as intended as-is.

Maybe @LanieOkorodudu or others can double-check my testing next week before we merge this!

@Geoffsc

Geoffsc commented Oct 18, 2024

Copy link
Copy Markdown
Contributor Author

@demiankatz Sounds good!

@LanieOkorodudu

Copy link
Copy Markdown
Contributor

@demiankatz, I have tested the enhancement, and it works as expected. The close button now correctly receives focus when clicked, ensuring that users are visually aware of the active focus state on the button. Thanks @Geoffsc

@demiankatz demiankatz merged commit 97b1e24 into UniversalViewer:dev Oct 21, 2024
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.

4 participants