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

Fix vertical option check on volumeMenuButton #2352

Closed
wants to merge 3 commits into from

Conversation

mmcc
Copy link
Member

@mmcc mmcc commented Jul 14, 2015

Previously, a horizontal volume menu button would not have worked. Even
once the vertical option was set to false (which was impossible before
due to an ill conceived || assignment), a lot of very specific styling
would have been required to make the menu look correct. This fixes both
issues by setting an orientation class on the parent menu button and
adding styling to account for both orientations.

Previously, a horizontal volume menu button would not have worked. Even
once the vertical option was set to false (which was impossible before
due to an ill conceived || assignment), a lot of very specific styling
would have been required to make the menu look correct. This fixes both
issues by setting an orientation class on the parent menu button and
adding styling to account for both orientations.
@mmcc mmcc added this to the v5.0.0 milestone Jul 14, 2015
@pam
Copy link

pam commented Jul 14, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 32b3972
Build details: https://travis-ci.org/pam/video.js/builds/70839648

(Please note that this is a fully automated comment.)

@mmcc
Copy link
Member Author

mmcc commented Jul 14, 2015

@pam retry

@pam
Copy link

pam commented Jul 14, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 32b3972
Build details: https://travis-ci.org/pam/video.js/builds/70859737

(Please note that this is a fully automated comment.)

@mmcc
Copy link
Member Author

mmcc commented Jul 14, 2015

@heff @dmlap Just so you guys know, I needed to do this to fix the horizontal orientation so I could start working on the inline expanding horizontal volume. If we go with that permanently (which is sounds like we are), then I'll switch this logic to make vertical: false the default.

constructor(player, options){
constructor(player, options={}){
// If the vertical option isn't passed at all, default to true.
if (options['vertical'] === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to quote options like this anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think so, I just kept doing it for consistency. Should we start just using dot notation at this point?

Copy link
Member

Choose a reason for hiding this comment

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

YES!

@dmlap
Copy link
Member

dmlap commented Jul 14, 2015

LGTM

@pam
Copy link

pam commented Jul 14, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: f2e4299
Build details: https://travis-ci.org/pam/video.js/builds/70970318

(Please note that this is a fully automated comment.)

@mmcc
Copy link
Member Author

mmcc commented Jul 14, 2015

@pam retry

@pam
Copy link

pam commented Jul 14, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: f2e4299
Build details: https://travis-ci.org/pam/video.js/builds/70972251

(Please note that this is a fully automated comment.)

*
* @method orientationClassName
*/
orientationClassName() {
Copy link
Member

Choose a reason for hiding this comment

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

Unless you're using this somewhere else you could probably just have this logic in the buildCSSClass function.

@heff
Copy link
Member

heff commented Jul 14, 2015

lgtm!

@pam
Copy link

pam commented Jul 15, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: c752fe3
Build details: https://travis-ci.org/pam/video.js/builds/71132634

(Please note that this is a fully automated comment.)

@mmcc mmcc closed this in e5d4757 Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants