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

Remove method Chaining from videojs #3860

Merged
merged 4 commits into from
Jan 18, 2017
Merged

Remove method Chaining from videojs #3860

merged 4 commits into from
Jan 18, 2017

Conversation

brandonocasey
Copy link
Contributor

Description

  • Remove method chaining from video.js
  • Returning a Promise from play should happen in a separate PR

Specific Changes proposed

List of methods that were changed:

  • src/js/component.js
    • ready
    • addClass
    • removeClass
    • toggleClass
    • show
    • hide
    • lockShowing
    • unlockShowing
    • setAttribute
    • removeAttribute
    • dimension - (Only when setting)
    • dimensions
    • on
    • off
    • one
    • trigger
    • height - (only when setting)
    • width - (only when setting)
  • src/js/modal-dialog.js
    • close
    • open
    • empty
    • fill
    • fillWith
  • src/js/slider/slider.js
    • vertical - (only when setting)
  • src/js/tech/tech.js
    • withSourceHandlers/setSource
  • src/js/clickable-component.js
    • controlText - getter
    • enable
    • disable
  • src/js/menu/menu-button.js
    • enable
    • disable
  • src/js/player.js
    • pause
    • play
    • scrubbing - (only when setting)
    • currentTime - (only when setting)
    • duration - (only when setting)
    • volume - (only when setting)
    • hasStarted - (only when setting)
    • muted - (only when setting)
    • isFullscreen - (only when setting)
    • requestFullscreen
    • exitFullscreen
    • src - (only when setting)
    • load
    • reset
    • preload - (only when setting)
    • autoplay - (only when setting)
    • loop - (only when setting)
    • poster - (only when setting)
    • controls - (only when setting)
    • usingNativeControls - (only when setting)
    • playbackRate - (only when setting)
    • isAudio - (only when setting)
    • language - (only when setting)
    • dimension - (only when setting)
    • error - (only when setting)
    • userActive - (only when setting)

Requirements Checklist

  • Feature implemented / Bug fixed
  • Change has been verified in an actual browser (Chome, Firefox, IE)
  • Unit Tests updated or fixed
  • Reviewed by Two Core Contributors

@brandonocasey brandonocasey added major needs: LGTM Needs one or more additional approvals labels Dec 14, 2016
@misteroneill misteroneill added confirmed and removed needs: LGTM Needs one or more additional approvals labels Dec 19, 2016
*/
open() {
if (!this.opened_) {
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't been better to not refactor this method and just remove the chaining capabilities for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will revert this refactor and add to another PR

@@ -592,7 +592,7 @@ class Player extends Component {
* The value to set the `Player's heigth to.
*
* @return {number}
* The current heigth of the `Player`.
* The current heigth of the `Player` when getting.
Copy link
Member

Choose a reason for hiding this comment

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

just realized there's a typo here: 'heigth' -> 'height'.

}

this[privDimension] = parsedVal;
}

this.updateStyleEl_();
return this;
return;
Copy link
Member

Choose a reason for hiding this comment

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

no need for this return.

don't refactor ModalDialog#open
don't return for no reason in Player#dimension
@brandonocasey
Copy link
Contributor Author

@gkatsev rebased, and code review changes addressed

@brandonocasey
Copy link
Contributor Author

removed method chaining changes from Player#play() due to #3907

@gkatsev
Copy link
Member

gkatsev commented Jan 18, 2017

Fixes #3704

@gkatsev gkatsev merged commit 8f07f5d into videojs:master Jan 18, 2017
@brandonocasey brandonocasey deleted the chore/remove-method-chaining branch February 2, 2017 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants