-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Refactoring chapters button handling and fixing several issues #3472
Changes from 12 commits
d19b811
bf01642
cfa817f
1f8d3d8
d608354
f4221f3
24db307
68741d2
891eea0
cc08ff7
6c1a5d7
8b69aef
6e3a8af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,8 +35,8 @@ | |
text-transform: lowercase; | ||
} | ||
|
||
.vjs-menu li:focus, | ||
.vjs-menu li:hover { | ||
.vjs-menu li.vjs-menu-item:focus, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why increase specificity? Can we drop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the menu title is a li too and I wanted to disable the hover effect there. Could have used the "not" selector but chose not to because of browser support. |
||
.vjs-menu li.vjs-menu-item:hover { | ||
outline: 0; | ||
@include background-color-with-alpha($secondary-background-color, $secondary-background-transparency); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,10 +3,7 @@ | |
*/ | ||
import TextTrackButton from './text-track-button.js'; | ||
import Component from '../../component.js'; | ||
import TextTrackMenuItem from './text-track-menu-item.js'; | ||
import ChaptersTrackMenuItem from './chapters-track-menu-item.js'; | ||
import Menu from '../../menu/menu.js'; | ||
import * as Dom from '../../utils/dom.js'; | ||
import toTitleCase from '../../utils/to-title-case.js'; | ||
|
||
/** | ||
|
@@ -37,108 +34,105 @@ class ChaptersButton extends TextTrackButton { | |
return `vjs-chapters-button ${super.buildCSSClass()}`; | ||
} | ||
|
||
/** | ||
* Create a menu item for each text track | ||
* | ||
* @return {Array} Array of menu items | ||
* @method createItems | ||
*/ | ||
createItems() { | ||
const items = []; | ||
const tracks = this.player_.textTracks(); | ||
update(event) { | ||
if (!this.track_ || (event && (event.type === 'addtrack' || event.type === 'removetrack'))) { | ||
this.setTrack(this.findChaptersTrack()); | ||
} | ||
super.update(); | ||
} | ||
|
||
if (!tracks) { | ||
return items; | ||
setTrack(track) { | ||
if (this.track_ === track) { | ||
return; | ||
} | ||
|
||
for (let i = 0; i < tracks.length; i++) { | ||
const track = tracks[i]; | ||
if (!this.updateHandler_) { | ||
this.updateHandler_ = this.update.bind(this); | ||
} | ||
|
||
if (track.kind === this.kind_) { | ||
items.push(new TextTrackMenuItem(this.player_, {track})); | ||
// here this.track_ refers to the old track instance | ||
if (this.track_) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a comment here that says that for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
const remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(this.track_); | ||
|
||
if (remoteTextTrackEl) { | ||
remoteTextTrackEl.removeEventListener('load', this.updateHandler_); | ||
} | ||
|
||
this.track_ = null; | ||
} | ||
|
||
return items; | ||
this.track_ = track; | ||
|
||
// here this.track_ refers to the new track instance | ||
if (this.track_) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and a comment here saying that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
this.track_.mode = 'hidden'; | ||
|
||
const remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(this.track_); | ||
|
||
if (remoteTextTrackEl) { | ||
remoteTextTrackEl.addEventListener('load', this.updateHandler_); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Create menu from chapter buttons | ||
* | ||
* @return {Menu} Menu of chapter buttons | ||
* @method createMenu | ||
*/ | ||
createMenu() { | ||
findChaptersTrack() { | ||
const tracks = this.player_.textTracks() || []; | ||
let chaptersTrack; | ||
let items = this.items || []; | ||
|
||
for (let i = tracks.length - 1; i >= 0; i--) { | ||
|
||
// We will always choose the last track as our chaptersTrack | ||
const track = tracks[i]; | ||
|
||
if (track.kind === this.kind_) { | ||
chaptersTrack = track; | ||
|
||
break; | ||
return track; | ||
} | ||
} | ||
} | ||
|
||
let menu = this.menu; | ||
|
||
if (menu === undefined) { | ||
menu = new Menu(this.player_); | ||
|
||
const title = Dom.createEl('li', { | ||
className: 'vjs-menu-title', | ||
innerHTML: toTitleCase(this.kind_), | ||
tabIndex: -1 | ||
}); | ||
|
||
menu.children_.unshift(title); | ||
Dom.insertElFirst(title, menu.contentEl()); | ||
} else { | ||
// We will empty out the menu children each time because we want a | ||
// fresh new menu child list each time | ||
items.forEach(item => menu.removeChild(item)); | ||
// Empty out the ChaptersButton menu items because we no longer need them | ||
items = []; | ||
getMenuCaption() { | ||
if (this.track_ && this.track_.label) { | ||
return this.track_.label; | ||
} | ||
return this.localize(toTitleCase(this.kind_)); | ||
} | ||
|
||
if (chaptersTrack && (chaptersTrack.cues === null || chaptersTrack.cues === undefined)) { | ||
chaptersTrack.mode = 'hidden'; | ||
/** | ||
* Create menu from chapter track | ||
* | ||
* @return {Menu} Menu of chapter buttons | ||
* @method createMenu | ||
*/ | ||
createMenu() { | ||
this.options_.title = this.getMenuCaption(); | ||
return super.createMenu(); | ||
} | ||
|
||
const remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(chaptersTrack); | ||
/** | ||
* Create a menu item for each chapter cue | ||
* | ||
* @return {Array} Array of menu items | ||
* @method createItems | ||
*/ | ||
createItems() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sometimes the diffs are so hard to read. I'll do another more thorough code review next week. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it was too big rewrite so diffs don't make too much sense here. Thank you, have a nice weekend. |
||
const items = []; | ||
|
||
if (remoteTextTrackEl) { | ||
remoteTextTrackEl.addEventListener('load', (event) => this.update()); | ||
} | ||
if (!this.track_) { | ||
return items; | ||
} | ||
|
||
if (chaptersTrack && chaptersTrack.cues && chaptersTrack.cues.length > 0) { | ||
const cues = chaptersTrack.cues; | ||
|
||
for (let i = 0, l = cues.length; i < l; i++) { | ||
const cue = cues[i]; | ||
const cues = this.track_.cues; | ||
|
||
const mi = new ChaptersTrackMenuItem(this.player_, { | ||
cue, | ||
track: chaptersTrack | ||
}); | ||
if (!cues) { | ||
return items; | ||
} | ||
|
||
items.push(mi); | ||
for (let i = 0, l = cues.length; i < l; i++) { | ||
const cue = cues[i]; | ||
const mi = new ChaptersTrackMenuItem(this.player_, { track: this.track_, cue }); | ||
|
||
menu.addChild(mi); | ||
} | ||
items.push(mi); | ||
} | ||
|
||
if (items.length > 0) { | ||
this.show(); | ||
} | ||
// Assigning the value of items back to this.items for next iteration | ||
this.items = items; | ||
return menu; | ||
return items; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want this change.