Fix vertical option check on volumeMenuButton #2352
Conversation
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.
|
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 (Please note that this is a fully automated comment.) |
|
@pam retry |
|
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 (Please note that this is a fully automated comment.) |
| constructor(player, options){ | ||
| constructor(player, options={}){ | ||
| // If the vertical option isn't passed at all, default to true. | ||
| if (options['vertical'] === undefined) { |
There was a problem hiding this comment.
Do you need to quote options like this anymore?
There was a problem hiding this comment.
No, I don't think so, I just kept doing it for consistency. Should we start just using dot notation at this point?
|
LGTM |
|
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 (Please note that this is a fully automated comment.) |
|
@pam retry |
|
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 (Please note that this is a fully automated comment.) |
| * | ||
| * @method orientationClassName | ||
| */ | ||
| orientationClassName() { |
There was a problem hiding this comment.
Unless you're using this somewhere else you could probably just have this logic in the buildCSSClass function.
|
lgtm! |
|
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 (Please note that this is a fully automated comment.) |
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 stylingwould 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.