-
Notifications
You must be signed in to change notification settings - Fork 864
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
Fix/ Improve accessibility of playlist icon buttons #4943
Fix/ Improve accessibility of playlist icon buttons #4943
Conversation
.playlistIcon:hover { | ||
background-color: var(--side-nav-hover-color); | ||
color: var(--side-nav-hover-text-color); | ||
stroke-width: 10; | ||
stroke: var(--side-nav-hover-text-color); | ||
transition: background 0.2s ease-in; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.playlistIcon:hover { | |
background-color: var(--side-nav-hover-color); | |
color: var(--side-nav-hover-text-color); | |
stroke-width: 10; | |
stroke: var(--side-nav-hover-text-color); | |
transition: background 0.2s ease-in; | |
} | |
.playlistIcon:hover { | |
background-color: var(--side-nav-hover-color); | |
color: var(--side-nav-hover-text-color); | |
transition: background 0.2s ease-in; | |
} |
This looks great @sossost, thanks for doing this so quickly! My only suggestion is that we keep the inactive hover state the same, as otherwise this makes it seem like it's active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, @jasonhenriquez! I appreciate your quick review. I agree with keeping the inactive hover state unchanged to avoid confusion about its activity status.
In this case, need to change the stroke color to the hover background color without deleting the style from the hover properties.
I used stroke color to make the icons thin without changing them.
I'll make the necessary adjustments and update the pull request shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! However, It's now not keeping the wider stoke width on active hover. You can add an additional rule for .playlistIconActive:hover
. I think it's a mistake in the existing implementation that the active color is lost on hover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! I missed updating the stroke width for the active hover state. I'll add a rule for .playlistIconActive:hover
to ensure the wider stroke width and active color are maintained during hover. I'll make this change and update the pull request shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sossost! This is working perfectly now. My last thought is that while we're here, can you make the .playlistIconActive:hover
's stroke
& color
be --accent-color-hover
? I think this was another pre-existing bug I didn't even notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ jasonhenriquez! I'm glad to hear it's working well now. I agree with your suggestion to update the .playlistIconActive:hover
stroke and color to --accent-color-hover
. It sounds like a good catch on a pre-existing issue. I'll make these changes and push the update soon.
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff. This looks much better to use and is now far more accessible to our disabled users as well. LGTM
@jasonhenriquez Thank you so much for taking the time to review and for your positive feedback! I'm happy to hear that the changes improve usability and accessibility. I love working together to improve the experience for all our users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this @sossost :)
Title
Improve accessibility of playlist icon buttons
Pull Request Type
Related issue
closes #4940
Description
The aria-pressed attribute has been added to playlist icon buttons.
In addition, the line thickness when the icon is deactivated has been adjusted to be slightly thinner, and when activated, the line thickness has been adjusted to be slightly thicker, so that it is possible to tell whether the icon is activated through not only the color but also the thickness of the icon.
Screenshots
before
after
Testing
Desktop
Additional context