From 8f07f5d57c52f68bf19d4dbbdde5725aafca3c31 Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Wed, 18 Jan 2017 01:50:22 -0500 Subject: [PATCH] refactor: Remove method Chaining from videojs (#3860) Remove method chaining from videojs. This helps keep the methods consistent especially since play() now returns a Promise in some cases. Also, it can add some performance benefits. BREAKING CHANGE: player methods no longer return a player instance when called. Fixes #3704. --- src/js/clickable-component.js | 13 +-- src/js/component.js | 85 ++-------------- src/js/menu/menu-button.js | 11 +- src/js/modal-dialog.js | 107 ++++++++------------ src/js/player.js | 179 ++++++++++++--------------------- src/js/slider/slider.js | 7 +- src/js/tech/tech.js | 5 - test/unit/component.test.js | 4 +- test/unit/modal-dialog.test.js | 93 ++++++++--------- 9 files changed, 174 insertions(+), 330 deletions(-) diff --git a/src/js/clickable-component.js b/src/js/clickable-component.js index 0e03856674..dbaa471c70 100644 --- a/src/js/clickable-component.js +++ b/src/js/clickable-component.js @@ -108,9 +108,8 @@ class ClickableComponent extends Component { * @param {Element} [el=this.el()] * Element to set the title on. * - * @return {string|ClickableComponent} + * @return {string} * - The control text when getting - * - Returns itself when setting; method can be chained. */ controlText(text, el = this.el()) { if (!text) { @@ -122,8 +121,6 @@ class ClickableComponent extends Component { this.controlText_ = text; this.controlTextEl_.innerHTML = localizedText; el.setAttribute('title', localizedText); - - return this; } /** @@ -138,9 +135,6 @@ class ClickableComponent extends Component { /** * Enable this `Component`s element. - * - * @return {ClickableComponent} - * Returns itself; method can be chained. */ enable() { this.removeClass('vjs-disabled'); @@ -152,14 +146,10 @@ class ClickableComponent extends Component { this.on('click', this.handleClick); this.on('focus', this.handleFocus); this.on('blur', this.handleBlur); - return this; } /** * Disable this `Component`s element. - * - * @return {ClickableComponent} - * Returns itself; method can be chained. */ disable() { this.addClass('vjs-disabled'); @@ -171,7 +161,6 @@ class ClickableComponent extends Component { this.off('click', this.handleClick); this.off('focus', this.handleFocus); this.off('blur', this.handleBlur); - return this; } /** diff --git a/src/js/component.js b/src/js/component.js index c05e542a4c..5517592c43 100644 --- a/src/js/component.js +++ b/src/js/component.js @@ -573,9 +573,6 @@ class Component { * The event handler if `first` is a `Component` and `second` is an event name * or an Array of event names. * - * @return {Component} - * Returns itself; method can be chained. - * * @listens Component#dispose */ on(first, second, third) { @@ -618,8 +615,6 @@ class Component { target.on('dispose', cleanRemover); } } - - return this; } /** @@ -635,9 +630,6 @@ class Component { * @param {EventTarget~EventListener} [third] * The event handler if `first` is a `Component` and `second` is an event name * or an Array of event names. - * - * @return {Component} - * Returns itself; method can be chained. */ off(first, second, third) { if (!first || typeof first === 'string' || Array.isArray(first)) { @@ -662,8 +654,6 @@ class Component { target.off('dispose', fn); } } - - return this; } /** @@ -678,9 +668,6 @@ class Component { * @param {EventTarget~EventListener} [third] * The event handler if `first` is a `Component` and `second` is an event name * or an Array of event names. - * - * @return {Component} - * Returns itself; method can be chained. */ one(first, second, third) { if (typeof first === 'string' || Array.isArray(first)) { @@ -700,8 +687,6 @@ class Component { this.on(target, type, newFunc); } - - return this; } /** @@ -713,13 +698,9 @@ class Component { * * @param {Object} [hash] * Data hash to pass along with the event - * - * @return {Component} - * Returns itself; method can be chained. */ trigger(event, hash) { Events.trigger(this.el_, event, hash); - return this; } /** @@ -731,9 +712,6 @@ class Component { * * @param {boolean} [sync=false] * Execute the listener synchronously if `Component` is ready. - * - * @return {Component} - * Returns itself; method can be chained. */ ready(fn, sync = false) { if (fn) { @@ -749,7 +727,6 @@ class Component { this.readyQueue_.push(fn); } } - return this; } /** @@ -847,13 +824,9 @@ class Component { * * @param {string} classToAdd * CSS class name to add - * - * @return {Component} - * Returns itself; method can be chained. */ addClass(classToAdd) { Dom.addElClass(this.el_, classToAdd); - return this; } /** @@ -861,13 +834,9 @@ class Component { * * @param {string} classToRemove * CSS class name to remove - * - * @return {Component} - * Returns itself; method can be chained. */ removeClass(classToRemove) { Dom.removeElClass(this.el_, classToRemove); - return this; } /** @@ -880,65 +849,45 @@ class Component { * * @param {boolean|Dom~predicate} [predicate] * An {@link Dom~predicate} function or a boolean - * - * @return {Component} - * Returns itself; method can be chained. */ toggleClass(classToToggle, predicate) { Dom.toggleElClass(this.el_, classToToggle, predicate); - return this; } /** * Show the `Component`s element if it is hidden by removing the * 'vjs-hidden' class name from it. - * - * @return {Component} - * Returns itself; method can be chained. */ show() { this.removeClass('vjs-hidden'); - return this; } /** * Hide the `Component`s element if it is currently showing by adding the * 'vjs-hidden` class name to it. - * - * @return {Component} - * Returns itself; method can be chained. */ hide() { this.addClass('vjs-hidden'); - return this; } /** * Lock a `Component`s element in its visible state by adding the 'vjs-lock-showing' * class name to it. Used during fadeIn/fadeOut. * - * @return {Component} - * Returns itself; method can be chained. - * * @private */ lockShowing() { this.addClass('vjs-lock-showing'); - return this; } /** * Unlock a `Component`s element from its visible state by removing the 'vjs-lock-showing' * class name from it. Used during fadeIn/fadeOut. * - * @return {Component} - * Returns itself; method can be chained. - * * @private */ unlockShowing() { this.removeClass('vjs-lock-showing'); - return this; } /** @@ -969,14 +918,10 @@ class Component { * @param {string} value * Value to set the attribute to. * - * @return {Component} - * Returns itself; method can be chained. - * * @see [DOM API]{@link https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute} */ setAttribute(attribute, value) { Dom.setAttribute(this.el_, attribute, value); - return this; } /** @@ -985,14 +930,10 @@ class Component { * @param {string} attribute * Name of the attribute to remove. * - * @return {Component} - * Returns itself; method can be chained. - * * @see [DOM API]{@link https://developer.mozilla.org/en-US/docs/Web/API/Element/removeAttribute} */ removeAttribute(attribute) { Dom.removeAttribute(this.el_, attribute); - return this; } /** @@ -1005,10 +946,9 @@ class Component { * @param {boolean} [skipListeners] * Skip the resize event trigger * - * @return {Component|number|string} - * - The width when getting, zero if there is no width. Can be a string + * @return {number|string} + * The width when getting, zero if there is no width. Can be a string * postpixed with '%' or 'px'. - * - Returns itself when setting; method can be chained. */ width(num, skipListeners) { return this.dimension('width', num, skipListeners); @@ -1024,10 +964,9 @@ class Component { * @param {boolean} [skipListeners] * Skip the resize event trigger * - * @return {Component|number|string} - * - The width when getting, zero if there is no width. Can be a string - * postpixed with '%' or 'px'. - * - Returns itself when setting; method can be chained. + * @return {number|string} + * The width when getting, zero if there is no width. Can be a string + * postpixed with '%' or 'px'. */ height(num, skipListeners) { return this.dimension('height', num, skipListeners); @@ -1041,13 +980,11 @@ class Component { * * @param {number|string} height * Height to set the `Component`s element to. - * - * @return {Component} - * Returns itself; method can be chained. */ dimensions(width, height) { // Skip resize listeners on width for optimization - return this.width(width, true).height(height); + this.width(width, true); + this.height(height); } /** @@ -1075,9 +1012,8 @@ class Component { * @param {boolean} [skipListeners] * Skip resize event trigger * - * @return {Component} - * - the dimension when getting or 0 if unset - * - Returns itself when setting; method can be chained. + * @return {number} + * The dimension when getting or 0 if unset */ dimension(widthOrHeight, num, skipListeners) { if (num !== undefined) { @@ -1106,8 +1042,7 @@ class Component { this.trigger('resize'); } - // Return component - return this; + return; } // Not setting a value, so getting it diff --git a/src/js/menu/menu-button.js b/src/js/menu/menu-button.js index 0936520915..2b061064c5 100644 --- a/src/js/menu/menu-button.js +++ b/src/js/menu/menu-button.js @@ -245,9 +245,6 @@ class MenuButton extends ClickableComponent { /** * Disable the `MenuButton`. Don't allow it to be clicked. - * - * @return {MenuButton} - * Returns itself; method can be chained. */ disable() { // Unpress, but don't force focus on this button @@ -257,19 +254,15 @@ class MenuButton extends ClickableComponent { this.enabled_ = false; - return super.disable(); + super.disable(); } /** * Enable the `MenuButton`. Allow it to be clicked. - * - * @return {MenuButton} - * Returns itself; method can be chained. */ enable() { this.enabled_ = true; - - return super.enable(); + super.enable(); } } diff --git a/src/js/modal-dialog.js b/src/js/modal-dialog.js index 598ac6a715..41adb3af36 100644 --- a/src/js/modal-dialog.js +++ b/src/js/modal-dialog.js @@ -153,20 +153,17 @@ class ModalDialog extends Component { * * @fires ModalDialog#beforemodalopen * @fires ModalDialog#modalopen - * - * @return {ModalDialog} - * Returns itself; method can be chained. */ open() { if (!this.opened_) { const player = this.player(); /** - * Fired just before a `ModalDialog` is opened. - * - * @event ModalDialog#beforemodalopen - * @type {EventTarget~Event} - */ + * Fired just before a `ModalDialog` is opened. + * + * @event ModalDialog#beforemodalopen + * @type {EventTarget~Event} + */ this.trigger('beforemodalopen'); this.opened_ = true; @@ -193,15 +190,14 @@ class ModalDialog extends Component { this.el().setAttribute('aria-hidden', 'false'); /** - * Fired just after a `ModalDialog` is opened. - * - * @event ModalDialog#modalopen - * @type {EventTarget~Event} - */ + * Fired just after a `ModalDialog` is opened. + * + * @event ModalDialog#modalopen + * @type {EventTarget~Event} + */ this.trigger('modalopen'); this.hasBeenOpened_ = true; } - return this; } /** @@ -226,48 +222,45 @@ class ModalDialog extends Component { * * @fires ModalDialog#beforemodalclose * @fires ModalDialog#modalclose - * - * @return {ModalDialog} - * Returns itself; method can be chained. */ close() { - if (this.opened_) { - const player = this.player(); + if (!this.opened_) { + return; + } + const player = this.player(); - /** - * Fired just before a `ModalDialog` is closed. - * - * @event ModalDialog#beforemodalclose - * @type {EventTarget~Event} - */ - this.trigger('beforemodalclose'); - this.opened_ = false; + /** + * Fired just before a `ModalDialog` is closed. + * + * @event ModalDialog#beforemodalclose + * @type {EventTarget~Event} + */ + this.trigger('beforemodalclose'); + this.opened_ = false; - if (this.wasPlaying_) { - player.play(); - } + if (this.wasPlaying_) { + player.play(); + } - if (this.closeable()) { - this.off(this.el_.ownerDocument, 'keydown', Fn.bind(this, this.handleKeyPress)); - } + if (this.closeable()) { + this.off(this.el_.ownerDocument, 'keydown', Fn.bind(this, this.handleKeyPress)); + } - player.controls(true); - this.hide(); - this.el().setAttribute('aria-hidden', 'true'); + player.controls(true); + this.hide(); + this.el().setAttribute('aria-hidden', 'true'); - /** - * Fired just after a `ModalDialog` is closed. - * - * @event ModalDialog#modalclose - * @type {EventTarget~Event} - */ - this.trigger('modalclose'); - - if (this.options_.temporary) { - this.dispose(); - } + /** + * Fired just after a `ModalDialog` is closed. + * + * @event ModalDialog#modalclose + * @type {EventTarget~Event} + */ + this.trigger('modalclose'); + + if (this.options_.temporary) { + this.dispose(); } - return this; } /** @@ -310,12 +303,9 @@ class ModalDialog extends Component { /** * Fill the modal's content element with the modal's "content" option. * The content element will be emptied before this change takes place. - * - * @return {ModalDialog} - * Returns itself; method can be chained. */ fill() { - return this.fillWith(this.content()); + this.fillWith(this.content()); } /** @@ -325,11 +315,8 @@ class ModalDialog extends Component { * @fires ModalDialog#beforemodalfill * @fires ModalDialog#modalfill * - * @param {Mixed} [content] - * The same rules apply to this as apply to the `content` option. - * - * @return {ModalDialog} - * Returns itself; method can be chained. + * @param {Mixed} [content] + * The same rules apply to this as apply to the `content` option. */ fillWith(content) { const contentEl = this.contentEl(); @@ -364,8 +351,6 @@ class ModalDialog extends Component { } else { parentEl.appendChild(contentEl); } - - return this; } /** @@ -373,9 +358,6 @@ class ModalDialog extends Component { * * @fires ModalDialog#beforemodalempty * @fires ModalDialog#modalempty - * - * @return {ModalDialog} - * Returns itself; method can be chained. */ empty() { /** @@ -394,7 +376,6 @@ class ModalDialog extends Component { * @type {EventTarget~Event} */ this.trigger('modalempty'); - return this; } /** diff --git a/src/js/player.js b/src/js/player.js index c2df80ce2b..909098d80c 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -568,7 +568,7 @@ class Player extends Component { * The value to set the `Player's width to. * * @return {number} - * The current width of the `Player`. + * The current width of the `Player` when getting. */ width(value) { return this.dimension('width', value); @@ -581,7 +581,7 @@ class Player extends Component { * The value to set the `Player's heigth to. * * @return {number} - * The current heigth of the `Player`. + * The current height of the `Player` when getting. */ height(value) { return this.dimension('height', value); @@ -598,9 +598,8 @@ class Player extends Component { * @param {number} [value] * Value for dimension specified in the first argument. * - * @return {Player|number} - * - Returns itself when setting; method can be chained. - * - The dimension arguments value when getting (width/height). + * @return {number} + * The dimension arguments value when getting (width/height). */ dimension(dimension, value) { const privDimension = dimension + '_'; @@ -617,14 +616,13 @@ class Player extends Component { if (isNaN(parsedVal)) { log.error(`Improper value "${value}" supplied for for ${dimension}`); - return this; + return; } this[privDimension] = parsedVal; } this.updateStyleEl_(); - return this; } /** @@ -1098,7 +1096,7 @@ class Player extends Component { this.removeClass('vjs-has-started'); } } - return this; + return; } return !!this.hasStarted_; } @@ -1594,7 +1592,6 @@ class Player extends Component { */ pause() { this.techCall_('pause'); - return this; } /** @@ -1617,24 +1614,20 @@ class Player extends Component { * @param {boolean} [isScrubbing] * wether the user is or is not scrubbing * - * @return {boolean|Player} - * A instance of the player that called this function when setting, - * and the value of scrubbing when getting + * @return {boolean} + * The value of scrubbing when getting */ scrubbing(isScrubbing) { - if (isScrubbing !== undefined) { - this.scrubbing_ = !!isScrubbing; - - if (isScrubbing) { - this.addClass('vjs-scrubbing'); - } else { - this.removeClass('vjs-scrubbing'); - } - - return this; + if (typeof isScrubbing === 'undefined') { + return this.scrubbing_; } + this.scrubbing_ = !!isScrubbing; - return this.scrubbing_; + if (isScrubbing) { + this.addClass('vjs-scrubbing'); + } else { + this.removeClass('vjs-scrubbing'); + } } /** @@ -1643,16 +1636,13 @@ class Player extends Component { * @param {number|string} [seconds] * The time to seek to in seconds * - * @return {Player|number} + * @return {number} * - the current time in seconds when getting - * - a reference to the current player object when setting */ currentTime(seconds) { - if (seconds !== undefined) { - + if (typeof seconds !== 'undefined') { this.techCall_('setCurrentTime', seconds); - - return this; + return; } // cache last currentTime and return. default to 0 seconds @@ -1678,10 +1668,8 @@ class Player extends Component { * @param {number} [seconds] * The duration of the video to set in seconds * - * @return {number|Player} + * @return {number} * - The duration of the video in seconds when getting - * - A reference to the player that called this function - * when setting */ duration(seconds) { if (seconds === undefined) { @@ -1710,8 +1698,6 @@ class Player extends Component { */ this.trigger('durationchange'); } - - return this; } /** @@ -1788,9 +1774,8 @@ class Player extends Component { * - 1.0 is 100%/full * - 0.5 is half volume or 50% * - * @return {Player|number} - * a reference to the calling player when setting and the - * current volume as a percent when getting + * @return {number} + * The current volume as a percent when getting */ volume(percentAsDecimal) { let vol; @@ -1801,7 +1786,7 @@ class Player extends Component { this.cache_.volume = vol; this.techCall_('setVolume', vol); - return this; + return; } // Default to 1 when returning current volume. @@ -1816,15 +1801,14 @@ class Player extends Component { * - true to mute * - false to unmute * - * @return {boolean|Player} + * @return {boolean} * - true if mute is on and getting * - false if mute is off and getting - * - A reference to the current player when setting */ muted(muted) { if (muted !== undefined) { this.techCall_('setMuted', muted); - return this; + return; } return this.techGet_('muted') || false; } @@ -1851,15 +1835,14 @@ class Player extends Component { * @param {boolean} [isFS] * Set the players current fullscreen state * - * @return {boolean|Player} + * @return {boolean} * - true if fullscreen is on and getting * - false if fullscreen is off and getting - * - A reference to the current player when setting */ isFullscreen(isFS) { if (isFS !== undefined) { this.isFullscreen_ = !!isFS; - return this; + return; } return !!this.isFullscreen_; } @@ -1874,8 +1857,6 @@ class Player extends Component { * Safari. * * @fires Player#fullscreenchange - * @return {Player} - * A reference to the current player */ requestFullscreen() { const fsApi = FullscreenApi; @@ -1921,17 +1902,12 @@ class Player extends Component { */ this.trigger('fullscreenchange'); } - - return this; } /** * Return the video to its normal size after having been in full screen mode * * @fires Player#fullscreenchange - * - * @return {Player} - * A reference to the current player */ exitFullscreen() { const fsApi = FullscreenApi; @@ -1951,8 +1927,6 @@ class Player extends Component { */ this.trigger('fullscreenchange'); } - - return this; } /** @@ -2152,9 +2126,8 @@ class Player extends Component { * @param {Tech~SourceObject|Tech~SourceObject[]} [source] * One SourceObject or an array of SourceObjects * - * @return {string|Player} - * - The current video source when getting - * - The player when setting + * @return {string} + * The current video source when getting */ src(source) { if (source === undefined) { @@ -2218,8 +2191,6 @@ class Player extends Component { }, true); } } - - return this; } /** @@ -2257,26 +2228,18 @@ class Player extends Component { /** * Begin loading the src data. - * - * @return {Player} - * A reference to the player */ load() { this.techCall_('load'); - return this; } /** * Reset the player. Loads the first tech in the techOrder, * and calls `reset` on the tech`. - * - * @return {Player} - * A reference to the player */ reset() { this.loadTech_(toTitleCase(this.options_.techOrder[0]), null); this.techCall_('reset'); - return this; } /** @@ -2344,15 +2307,14 @@ class Player extends Component { * - true means that we should preload * - false maens that we should not preload * - * @return {string|Player} - * - the preload attribute value when getting - * - the player when setting + * @return {string} + * The preload attribute value when getting */ preload(value) { if (value !== undefined) { this.techCall_('setPreload', value); this.options_.preload = value; - return this; + return; } return this.techGet_('preload'); } @@ -2362,17 +2324,16 @@ class Player extends Component { * * @param {boolean} [value] * - true means that we should autoplay - * - false maens that we should not autoplay + * - false means that we should not autoplay * - * @return {string|Player} - * - the current value of autoplay - * - the player when setting + * @return {string} + * The current value of autoplay when getting */ autoplay(value) { if (value !== undefined) { this.techCall_('setAutoplay', value); this.options_.autoplay = value; - return this; + return; } return this.techGet_('autoplay', value); } @@ -2384,15 +2345,14 @@ class Player extends Component { * - true means that we should loop the video * - false means that we should not loop the video * - * @return {string|Player} - * - the current value of loop when getting - * - the player when setting + * @return {string} + * The current value of loop when getting */ loop(value) { if (value !== undefined) { this.techCall_('setLoop', value); this.options_.loop = value; - return this; + return; } return this.techGet_('loop'); } @@ -2405,9 +2365,8 @@ class Player extends Component { * @param {string} [src] * Poster image source URL * - * @return {string|Player} - * - the current value of poster when getting - * - the player when setting + * @return {string} + * The current value of poster when getting */ poster(src) { if (src === undefined) { @@ -2434,8 +2393,6 @@ class Player extends Component { * @type {EventTarget~Event} */ this.trigger('posterchange'); - - return this; } /** @@ -2468,9 +2425,8 @@ class Player extends Component { * - true to turn controls on * - false to turn controls off * - * @return {boolean|Player} - * - the current value of controls when getting - * - the player when setting + * @return {boolean} + * The current value of controls when getting */ controls(bool) { if (bool !== undefined) { @@ -2510,7 +2466,7 @@ class Player extends Component { } } } - return this; + return; } return !!this.controls_; } @@ -2529,9 +2485,8 @@ class Player extends Component { * - true to turn native controls on * - false to turn native controls off * - * @return {boolean|Player} - * - the current value of native controls when getting - * - the player when setting + * @return {boolean} + * The current value of native controls when getting */ usingNativeControls(bool) { if (bool !== undefined) { @@ -2562,7 +2517,7 @@ class Player extends Component { this.trigger('usingcustomcontrols'); } } - return this; + return; } return !!this.usingNativeControls_; } @@ -2576,9 +2531,8 @@ class Player extends Component { * A MediaError or a string/number to be turned * into a MediaError * - * @return {MediaError|null|Player} - * - The current MediaError when getting (or null) - * - The player when setting + * @return {MediaError|null} + * The current MediaError when getting (or null) */ error(err) { if (err === undefined) { @@ -2592,7 +2546,7 @@ class Player extends Component { if (this.errorDisplay) { this.errorDisplay.close(); } - return this; + return; } this.error_ = new MediaError(err); @@ -2610,7 +2564,7 @@ class Player extends Component { */ this.trigger('error'); - return this; + return; } /** @@ -2632,9 +2586,9 @@ class Player extends Component { * @param {boolean} [bool] * - true if the user is active * - false if the user is inactive - * @return {boolean|Player} - * - the current value of userActive when getting - * - the player when setting + * + * @return {boolean} + * The current value of userActive when getting */ userActive(bool) { if (bool !== undefined) { @@ -2681,7 +2635,7 @@ class Player extends Component { this.trigger('userinactive'); } } - return this; + return; } return this.userActive_; } @@ -2782,14 +2736,13 @@ class Player extends Component { * @param {number} [rate] * New playback rate to set. * - * @return {number|Player} - * - The current playback rate when getting or 1.0 - * - the player when setting + * @return {number} + * The current playback rate when getting or 1.0 */ playbackRate(rate) { if (rate !== undefined) { this.techCall_('setPlaybackRate', rate); - return this; + return; } if (this.tech_ && this.tech_.featuresPlaybackRate) { @@ -2805,14 +2758,13 @@ class Player extends Component { * - true signals that this is an audio player * - false signals that this is not an audio player * - * @return {Player|boolean} - * - the current value of isAudio when getting - * - the player if setting + * @return {boolean} + * The current value of isAudio when getting */ isAudio(bool) { if (bool !== undefined) { this.isAudio_ = !!bool; - return this; + return; } return !!this.isAudio_; @@ -3021,9 +2973,8 @@ class Player extends Component { * @param {string} [code] * the language code to set the player to * - * @return {string|Player} - * - The current language code when getting - * - A reference to the player when setting + * @return {string} + * The current language code when getting */ language(code) { if (code === undefined) { @@ -3031,7 +2982,6 @@ class Player extends Component { } this.language_ = String(code).toLowerCase(); - return this; } /** @@ -3098,7 +3048,8 @@ class Player extends Component { this.removeChild(modal); }); - return modal.open(); + modal.open(); + return modal; } /** diff --git a/src/js/slider/slider.js b/src/js/slider/slider.js index 11f7b4eaf6..0dee6f3519 100644 --- a/src/js/slider/slider.js +++ b/src/js/slider/slider.js @@ -281,10 +281,9 @@ class Slider extends Component { * - true if slider is vertical, * - false is horizontal * - * @return {boolean|Slider} + * @return {boolean} * - true if slider is vertical, and getting - * - false is horizontal, and getting - * - a reference to this object when setting + * - false if the slider is horizontal, and getting */ vertical(bool) { if (bool === undefined) { @@ -298,8 +297,6 @@ class Slider extends Component { } else { this.addClass('vjs-slider-horizontal'); } - - return this; } } diff --git a/src/js/tech/tech.js b/src/js/tech/tech.js index 48e07a919d..8695a522a1 100644 --- a/src/js/tech/tech.js +++ b/src/js/tech/tech.js @@ -1118,9 +1118,6 @@ Tech.withSourceHandlers = function(_Tech) { * * @param {Tech~SourceObject} source * A source object with src and type keys - * - * @return {Tech} - * Returns itself; this method is chainable */ _Tech.prototype.setSource = function(source) { let sh = _Tech.selectSourceHandler(source, this.options_); @@ -1145,8 +1142,6 @@ Tech.withSourceHandlers = function(_Tech) { this.sourceHandler_ = sh.handleSource(source, this, this.options_); this.on('dispose', this.disposeSourceHandler); - - return this; }; /** diff --git a/test/unit/component.test.js b/test/unit/component.test.js index cd011910a9..43c706efcf 100644 --- a/test/unit/component.test.js +++ b/test/unit/component.test.js @@ -595,13 +595,13 @@ QUnit.test('dimension() should treat NaN and null as zero', function(assert) { newWidth = comp.dimension('width', null); assert.notEqual(newWidth, width, 'new width and old width are not the same'); - assert.equal(newWidth, comp, 'we set a value, so, return value is component'); + assert.equal(newWidth, undefined, 'we set a value, so, return value is undefined'); assert.equal(comp.width(), 0, 'the new width is zero'); const newHeight = comp.dimension('height', NaN); assert.notEqual(newHeight, height, 'new height and old height are not the same'); - assert.equal(newHeight, comp, 'we set a value, so, return value is component'); + assert.equal(newHeight, undefined, 'we set a value, so, return value is undefined'); assert.equal(comp.height(), 0, 'the new height is zero'); comp.width(width); diff --git a/test/unit/modal-dialog.test.js b/test/unit/modal-dialog.test.js index 8d094515f2..5ef9cebc92 100644 --- a/test/unit/modal-dialog.test.js +++ b/test/unit/modal-dialog.test.js @@ -87,15 +87,6 @@ QUnit.test('should create a close button by default', function(assert) { assert.strictEqual(btn.el().parentNode, this.el, 'close button is a child of el'); }); -QUnit.test('returns `this` for expected methods', function(assert) { - const methods = ['close', 'empty', 'fill', 'fillWith', 'open']; - - assert.expect(methods.length); - methods.forEach(function(method) { - assert.strictEqual(this[method](), this, '`' + method + '()` returns `this`'); - }, this.modal); -}); - QUnit.test('open() triggers events', function(assert) { const modal = this.modal; const beforeModalOpenSpy = sinon.spy(function() { @@ -108,10 +99,9 @@ QUnit.test('open() triggers events', function(assert) { assert.expect(4); - modal. - on('beforemodalopen', beforeModalOpenSpy). - on('modalopen', modalOpenSpy). - open(); + modal.on('beforemodalopen', beforeModalOpenSpy); + modal.on('modalopen', modalOpenSpy); + modal.open(); assert.strictEqual(beforeModalOpenSpy.callCount, 1, 'beforemodalopen spy was called'); assert.strictEqual(modalOpenSpy.callCount, 1, 'modalopen spy was called'); @@ -127,7 +117,9 @@ QUnit.test('open() removes "vjs-hidden" class', function(assert) { QUnit.test('open() cannot be called on an opened modal', function(assert) { const spy = sinon.spy(); - this.modal.on('modalopen', spy).open().open(); + this.modal.on('modalopen', spy); + this.modal.open(); + this.modal.open(); assert.expect(1); assert.strictEqual(spy.callCount, 1, 'modal was only opened once'); @@ -145,11 +137,10 @@ QUnit.test('close() triggers events', function(assert) { assert.expect(4); - modal. - on('beforemodalclose', beforeModalCloseSpy). - on('modalclose', modalCloseSpy). - open(). - close(); + modal.on('beforemodalclose', beforeModalCloseSpy); + modal.on('modalclose', modalCloseSpy); + modal.open(); + modal.close(); assert.strictEqual(beforeModalCloseSpy.callCount, 1, 'beforemodalclose spy was called'); assert.strictEqual(modalCloseSpy.callCount, 1, 'modalclose spy was called'); @@ -157,18 +148,21 @@ QUnit.test('close() triggers events', function(assert) { QUnit.test('close() adds the "vjs-hidden" class', function(assert) { assert.expect(1); - this.modal.open().close(); + this.modal.open(); + this.modal.close(); assert.ok(this.modal.hasClass('vjs-hidden'), 'modal is hidden upon close'); }); QUnit.test('pressing ESC triggers close(), but only when the modal is opened', function(assert) { const spy = sinon.spy(); - this.modal.on('modalclose', spy).handleKeyPress({which: ESC}); + this.modal.on('modalclose', spy); + this.modal.handleKeyPress({which: ESC}); assert.expect(2); assert.strictEqual(spy.callCount, 0, 'ESC did not close the closed modal'); - this.modal.open().handleKeyPress({which: ESC}); + this.modal.open(); + this.modal.handleKeyPress({which: ESC}); assert.strictEqual(spy.callCount, 1, 'ESC closed the now-opened modal'); }); @@ -176,7 +170,9 @@ QUnit.test('close() cannot be called on a closed modal', function(assert) { const spy = sinon.spy(); this.modal.on('modalclose', spy); - this.modal.open().close().close(); + this.modal.open(); + this.modal.close(); + this.modal.close(); assert.expect(1); assert.strictEqual(spy.callCount, 1, 'modal was only closed once'); @@ -227,11 +223,10 @@ QUnit.test('opened()', function(assert) { this.modal.open(); assert.strictEqual(this.modal.opened(), true, 'the modal is open'); - this.modal. - close(). - on('modalopen', openSpy). - on('modalclose', closeSpy). - opened(true); + this.modal.close(); + this.modal.on('modalopen', openSpy); + this.modal.on('modalclose', closeSpy); + this.modal.opened(true); this.modal.opened(true); this.modal.opened(false); @@ -260,10 +255,9 @@ QUnit.test('fillWith()', function(assert) { contentEl.appendChild(el); }); - this.modal. - on('beforemodalfill', beforeFillSpy). - on('modalfill', fillSpy). - fillWith(children); + this.modal.on('beforemodalfill', beforeFillSpy); + this.modal.on('modalfill', fillSpy); + this.modal.fillWith(children); assert.expect(5 + children.length); assert.strictEqual(contentEl.children.length, children.length, 'has the right number of children'); @@ -282,11 +276,10 @@ QUnit.test('empty()', function(assert) { const beforeEmptySpy = sinon.spy(); const emptySpy = sinon.spy(); - this.modal. - fillWith([Dom.createEl(), Dom.createEl()]). - on('beforemodalempty', beforeEmptySpy). - on('modalempty', emptySpy). - empty(); + this.modal.fillWith([Dom.createEl(), Dom.createEl()]); + this.modal.on('beforemodalempty', beforeEmptySpy); + this.modal.on('modalempty', emptySpy); + this.modal.empty(); assert.expect(5); assert.strictEqual(this.modal.contentEl().children.length, 0, 'removed all `contentEl()` children'); @@ -302,7 +295,8 @@ QUnit.test('closeable()', function(assert) { assert.expect(8); assert.strictEqual(this.modal.closeable(), true, 'the modal is closed'); - this.modal.open().closeable(false); + this.modal.open(); + this.modal.closeable(false); assert.notOk(this.modal.getChild('closeButton'), 'the close button is no longer a child of the modal'); assert.notOk(initialCloseButton.el(), 'the initial close button was disposed'); @@ -312,13 +306,15 @@ QUnit.test('closeable()', function(assert) { this.modal.close(); assert.notOk(this.modal.opened(), 'the modal was closed programmatically'); - this.modal.open().closeable(true); + this.modal.open(); + this.modal.closeable(true); assert.ok(this.modal.getChild('closeButton'), 'a new close button was created'); this.modal.getChild('closeButton').trigger('click'); assert.notOk(this.modal.opened(), 'the modal was closed by the new close button'); - this.modal.open().handleKeyPress({which: ESC}); + this.modal.open(); + this.modal.handleKeyPress({which: ESC}); assert.notOk(this.modal.opened(), 'the modal was closed by the ESC key'); }); @@ -331,7 +327,9 @@ QUnit.test('"content" option (fills on first open() invocation)', function(asser const spy = sinon.spy(); modal.on('modalfill', spy); - modal.open().close().open(); + modal.open(); + modal.close(); + modal.open(); assert.expect(3); assert.strictEqual(modal.content(), modal.options_.content, 'has the expected content'); @@ -347,8 +345,10 @@ QUnit.test('"temporary" option', function(assert) { temp.on('dispose', tempSpy); perm.on('dispose', permSpy); - temp.open().close(); - perm.open().close(); + temp.open(); + temp.close(); + perm.open(); + perm.close(); assert.expect(2); assert.strictEqual(tempSpy.callCount, 1, 'temporary modals are disposed'); @@ -365,7 +365,9 @@ QUnit.test('"fillAlways" option', function(assert) { const spy = sinon.spy(); modal.on('modalfill', spy); - modal.open().close().open(); + modal.open(); + modal.close(); + modal.open(); assert.expect(1); assert.strictEqual(spy.callCount, 2, 'the modal was filled on each open call'); @@ -393,6 +395,7 @@ QUnit.test('"uncloseable" option', function(assert) { assert.strictEqual(modal.closeable(), false, 'the modal is uncloseable'); assert.notOk(modal.getChild('closeButton'), 'the close button is not present'); - modal.open().handleKeyPress({which: ESC}); + modal.open(); + modal.handleKeyPress({which: ESC}); assert.strictEqual(spy.callCount, 0, 'ESC did not close the modal'); });