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

Apply hover styles also on keyboard focus #662

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Jan 17, 2023

This adds the :focus selector to almost all elements that feature a style that is being applied on hover events. This should improve the usability of keyboard-only navigation. Exceptions are elements that are not keyboard-focusable, like the "logged in as " and upload date on video pages.

Closes #649

This adds the `:focus` selector to almost all elements
that feature a style that is being applied on hover events.
This should improve the usability of keyboard-only navigation.
Exceptions are elements that are not keyboard-focusable,
like the "logged in as <username>" and upload date on
video pages.
@owi92 owi92 added the changelog:user User facing changes label Jan 17, 2023
@lkiesow
Copy link
Contributor

lkiesow commented Jan 17, 2023

Tested this using the deployment: Looks good. I didn't notice any large problems. It definitely fixes the problematic buttons I noticed before.

@owi92
Copy link
Member Author

owi92 commented Jan 17, 2023

Thank you for testing! Just to clarify, the issue described in #649 is fixed by line 48 in frontend/src/ui/Button.tsx. But I figured it would make sense to also unify other hover and focus styles in this PR, as that is quite simple to do as you can see. I also tested everything I changed and would not expect that this introduces any problems.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Looks good, the comments I left are nothing that needs to be fixed!

frontend/src/routes/manage/Video/Shared.tsx Show resolved Hide resolved
@@ -175,7 +175,7 @@ const EventTable: React.FC<EventTableProps> = ({ events, vars }) => {
},
},
"& > tbody": {
"& > tr:hover": {
"& > tr:hover, tr:focus-within": {
Copy link
Member

Choose a reason for hiding this comment

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

I think you are missing a & > here? It seems to work anyway. Mh. I have the strong feeling that most & I used are not necessary :D

Suggested change
"& > tr:hover, tr:focus-within": {
"& > tr:hover, & > tr:focus-within": {

@LukasKalbertodt LukasKalbertodt merged commit 8d05045 into elan-ev:master Jan 17, 2023
@owi92 owi92 deleted the unify-hover-and-focus-styles branch March 4, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different styles for activating the same button
3 participants