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

Made a few volume skin changes #2553

Closed
wants to merge 5 commits into from
Closed

Conversation

heff
Copy link
Member

@heff heff commented Sep 4, 2015

Related to #2507

  • Removed the non-default volume controls from the control bar
  • Made the volume default to inline
  • Started fixing the inline volume
    • Fixed hovering out of the slider while sliding (it would close on you)
    • Extended the hover area
    • There were a bunch of issues around tab focusing on the volume

Still need to figure out transitions on tab navigation. Looks a little wonky. Need @mmcc to check it out.

UPDATE: Also there seems to be something off with Safari's tab navigation now

- Removed the non-default volume controls from the control bar
- Made the volume default to inline
- Started fixing the inline volume
@heff heff added this to the v5.0.0 milestone Sep 4, 2015
@@ -44,12 +44,14 @@
}

// Hover state
.video-js .vjs-menu-button-inline:hover {
.video-js .vjs-menu-button-inline:hover,
.video-js .vjs-menu-button-inline.vjs-slider-active {
Copy link
Member

Choose a reason for hiding this comment

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

why not just use :focus (or :active)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The menu button isn't actually in focus here, the internal slider is. I have the slider firing an event when it's active so the menu button can catch it add this class to itself.

@heff
Copy link
Member Author

heff commented Sep 5, 2015

All the volume in all their glory.

screen shot 2015-09-04 at 5 12 37 pm

Surprisingly working in IE8 too. I forced the inline volume to just be always showing in IE8. It might be possible to make it work the same but not sure it's worth the effort.

I also did a pass on reducing the selector strength on the involved components here and was able to clean up a bunch.

Could use another review.

@@ -46,7 +46,7 @@ ControlBar.prototype.options_ = {
loadEvent: 'play',
children: [
'playToggle',
'volumeMenuButton',
{ name: 'volumeMenuButton', inline: true },
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I should switch the default of the volume menu button to inline

@gkatsev
Copy link
Member

gkatsev commented Sep 8, 2015

I like the removal of specificity in the selectors. Seems like good changes.
LGTM.

@heff
Copy link
Member Author

heff commented Sep 8, 2015

Awesome, thanks!

@heff heff closed this in 4dadd01 Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants