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.

Sign up for GitHub

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

Jump to bottom

Making sure that the title element gets placed into the UL element #1114

Merged
merged 1 commit into from
Apr 3, 2014
Merged

Making sure that the title element gets placed into the UL element #1114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/js/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ vjs.MenuButton.prototype.createMenu = function(){

// Add a title list item to the top
if (this.options().title) {
menu.el().appendChild(vjs.createEl('li', {
menu.contentEl().appendChild(vjs.createEl('li', {
className: 'vjs-menu-title',
innerHTML: vjs.capitalize(this.kind_),
Copy link
Member

Choose a reason for hiding this comment

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

I found some other related issues after pulling this in. For instance, this.kind_ here should really be this.options().title. Kind must have gotten copied over from tracks at some point.

tabindex: -1
Expand Down
2 changes: 1 addition & 1 deletion src/js/tracks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,7 @@ vjs.ChaptersButton.prototype.createMenu = function(){

var menu = this.menu = new vjs.Menu(this.player_);

menu.el_.appendChild(vjs.createEl('li', {
menu.contentEl().appendChild(vjs.createEl('li', {
className: 'vjs-menu-title',
innerHTML: vjs.capitalize(this.kind_),
tabindex: -1
Expand Down
4 changes: 3 additions & 1 deletion test/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
'test/unit/poster.js',
'test/unit/plugins.js',
'test/unit/flash.js',
'test/unit/api.js'
'test/unit/api.js',
'test/unit/menu.js',
'test/unit/tracks.js'
];

var projectRoot = '../';
Expand Down
20 changes: 20 additions & 0 deletions test/unit/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
module('MenuButton');

test('should place title list item into ul', function() {
var player, menuButton;

player = PlayerTest.makePlayer();

vjs.MenuButton.prototype.kind_ = 'testTitle';

menuButton = new vjs.MenuButton(player, {
'title': true
});

var menuContentElement = menuButton.el().getElementsByTagName('UL')[0];
var titleElement = menuContentElement.children[0];

ok(titleElement.innerHTML === 'TestTitle', 'title element placed in ul');

player.dispose();
});
16 changes: 16 additions & 0 deletions test/unit/tracks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module('Tracks');

test('should place title list item into ul', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like there's an opportunity to de-dupe some code here, but we can worry about that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't know if the convention was to split by module or if i could have just thrown both of these tests in the same file

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the way you set it up is right. It just points to the fact that we have the exact same code in two places. You don't need to worry about that for this one.

var player, chaptersButton;

player = PlayerTest.makePlayer();

chaptersButton = new vjs.ChaptersButton(player);

var menuContentElement = chaptersButton.el().getElementsByTagName('UL')[0];
var titleElement = menuContentElement.children[0];

ok(titleElement.innerHTML === 'Chapters', 'title element placed in ul');

player.dispose();
});