-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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 chapters menu #3163
Fix chapters menu #3163
Conversation
FYI, there's no Chapters menu example in the existing repo, but there is one in the Descriptions example in #3098. |
Yep, for testing I just copied the standard oceans track and called it chapters. |
Also, looking into #3098 is what prompted me to fix this issue in the first place. |
Oh, great! I can take a look at this fix or yours this evening, and give you a LGTM. |
That would be great, thanks! |
@@ -90,11 +90,13 @@ class ChaptersButton extends TextTrackButton { | |||
let menu = this.menu; | |||
if (menu === undefined) { | |||
menu = new Menu(this.player_); | |||
menu.contentEl().appendChild(Dom.createEl('li', { | |||
let title = Dom.createEl('li', { |
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.
1/. Since this is a new Menu, what's the situation where it can have children already?
2/. meun-button.js already has this mechanism to add a title (using options.title) which doesn't seem to be used elsewhere - why isn't that mecanism used here?
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.
- It's unlikely that it should be a problem, especially since it should be the first item.
- I'll have to take a look at that and see whether it applies in this case or what its used for. Thanks.
A couple of comments, otherwise LGTM. It exposes a limitation of the keyboard handling of menus, though, which I'll need to fix. :-( |
@@ -70,11 +70,13 @@ class MenuButton extends ClickableComponent { | |||
|
|||
// Add a title list item to the top | |||
if (this.options_.title) { | |||
menu.contentEl().appendChild(Dom.createEl('li', { | |||
let title = Dom.createEl('li', { |
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.
The menu title should probably be a separate dive inside menu with the menu items being in a separate UL, but we can't make the change in a minor update.
LGTM. |
This is because of the change in videojs#3163 where we added the title as an item in the childrens array so that it is ordered correctly but this breaks keyboard support because they expect the children to be lined up correctly and also because they expect the children to be component objects with an 'el_' property. Checking to see if the first item is a 'vjs-menu-title' and adding one to the 'item' allows us to ignore the title element and also select the actual menu items.
Description
Fixes #3062. The
left
andwidth
properties for the chapters menu is unnecessary.Also, we need to make sure that the chapters title gets added in first into the childrens array so that it's always at the top.
Requirements Checklist