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: stop the vertical volume slider from showing up after mouse hover #5505

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@14willh
Copy link

14willh commented Oct 14, 2018

Description

Hovering over vertical volume slider when it has opacity: 0 (i.e. when it is hidden) triggers the volume slider to transition to opacity: 1 (i.e. shown), which is undesirable - it seems like there is a "ghost" element.

See #5502 for more

Could simply use display:none instead of opacity: 0, and display: flex instead of opacity: 1, but this would remove the transition animation.

Specific Changes proposed

In the _volume.scss file, this change/fix adds transform: scale(0) translate(0, 100%) property to the vertical volume control default state SCSS rule, and transform: scale(1) translate(0, 0) to its :hover state. These changes only affect the behavior of the vertical volume control - the horizontal volume control remains unchanged. transform 0.1s has been added to the corresponding $transition-property variables.

Additionally, scale and translate values have been chosen to make the :hover transition feel similar to the progress bar time tooltip :hover transition.

Have appended !important to transform: scale(1) translate(0, 0) to make this work. Not sure if this could be avoided by reordering some of the SCSS?

Fixes #5502.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors
fix: stop the vertical volume slider from showing up after mouse hove…
…ring on it

Hovering over vertical volume slider when it has opacity: 0 (i.e. when it is hidden) triggers the volume slider to transition to opacity: 1 (i.e. shown), which is undesirable - it seems like there is a "ghost" element. This change adds transform: scale(0) translate(0, 100%) property to the vertical volume control default state CSS rule, and transform: scale(1) translate(0, 0) to its :hover state. These changes only affect the behavior of the vertical volume control - the horizontal volume control remains unchanged. Furthermore, scale and translate values have been chosen to make the :hover animation feel similar to the progress bar time tooltip :hover animation. transform 0.1s has been added to the corresponding $transition-property variables.
Have added !important with the scale(1) translate(0, 0) transform to make this work. Not sure if this could be avoided by reordering some of the SCSS?
Fixes #5502.
@welcome

This comment has been minimized.

Copy link

welcome bot commented Oct 14, 2018

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@BrandonOCasey

This comment has been minimized.

Copy link
Contributor

BrandonOCasey commented Oct 25, 2018

I think this is a UI improvement, and it appears to work across the board. However, it current breaks the way that "tabbing" between controls works. You can no longer tab to the play-pause button and you can no longer tab to the mute toggle.

@14willh

This comment has been minimized.

Copy link
Author

14willh commented Oct 25, 2018

@BrandonOCasey Thanks for your feedback!

I can't recreate the tabbing problems you are describing. I have tested on my codepen example that has my improvements and I am able to tab back and forth between all the UI controls with both a vertical and horizontal volume slider. Tested on Chrome, Firefox, Edge. Can you provide any details on how I can recreate this?

@BrandonOCasey

This comment has been minimized.

Copy link
Contributor

BrandonOCasey commented Oct 25, 2018

Hey @14willh, I went back and tested it again, and your right everything does work as expected. It seems like I was seeing an issue with safari 12 but after talking to @gkatsev I was just doing it wrong.

@BrandonOCasey
Copy link
Contributor

BrandonOCasey left a comment

I tested that this works on IE 11, Safari, Edge, Chrome, and Firefox

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Oct 25, 2018

With this change, the animation is really different to other animations and to the current animation of it just fading out. There's another PR for this fix #5503 which keeps the current hiding animation.

@14willh

This comment has been minimized.

Copy link
Author

14willh commented Oct 29, 2018

@gkatsev I agree that there are inconsistencies between the transitions of each UI control - e.g. the playback rate menu doesn't have any transition. I'll have a play around to see if I can make them more unified without changing too much.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Oct 29, 2018

@14willh please check out #5503, if that works for you, maybe we can just go with that and then you won't need to do any extra work :)

@gkatsev gkatsev closed this in 7d127c8 Nov 5, 2018

@14willh

This comment has been minimized.

Copy link
Author

14willh commented Nov 6, 2018

@gkatsev yeah #5503 probably fits better, with less changes :) Thanks for looking at my solution anyway!

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Nov 6, 2018

Help is always appreciated. Thanks for the PR even if it didn't make it in.

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