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

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

Merged
merged 1 commit into from Apr 3, 2014

Conversation

Projects
None yet
2 participants
@jnwng
Member

jnwng commented Mar 26, 2014

Included are two sanity check tests for the two areas being checked
to make sure that the menu title items are being correctly placed
in the actual UL element of the menu.

Fixes #1107

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Mar 26, 2014

Member

Cool, thanks!

Member

heff commented Mar 26, 2014

Cool, thanks!

Show outdated Hide outdated test/unit/menu.js
});
var menuContentElement = menuButton.el().getElementsByTagName('UL')[0];
var titleElement = menuContentElement.getElementsByClassName(

This comment has been minimized.

@heff

heff Mar 26, 2014

Member

I think we'll need to use something other than getElementsByClassName to get tests to pass in IE8.

@heff

heff Mar 26, 2014

Member

I think we'll need to use something other than getElementsByClassName to get tests to pass in IE8.

This comment has been minimized.

@jnwng

jnwng Mar 26, 2014

Member

i can probably iterate over the children, the first one should be the one we want

@jnwng

jnwng Mar 26, 2014

Member

i can probably iterate over the children, the first one should be the one we want

This comment has been minimized.

@heff

heff Mar 31, 2014

Member

Yeah, that'd work.

@heff

heff Mar 31, 2014

Member

Yeah, that'd work.

@@ -0,0 +1,17 @@
module('Tracks');
test('should place title list item into ul', function() {

This comment has been minimized.

@heff

heff Mar 26, 2014

Member

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

@heff

heff Mar 26, 2014

Member

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

This comment has been minimized.

@jnwng

jnwng Mar 26, 2014

Member

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

@jnwng

jnwng Mar 26, 2014

Member

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

This comment has been minimized.

@heff

heff Mar 31, 2014

Member

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.

@heff

heff Mar 31, 2014

Member

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.

Making sure that the title element gets placed into the UL element
Included are two sanity check tests for the two areas being checked
to make sure that the menu title items are being correctly placed
in the actual UL element of the menu.

Fixes #1107
@jnwng

This comment has been minimized.

Show comment
Hide comment
@jnwng

jnwng Apr 2, 2014

Member

@heff updated to use the children property for the sake of ie8.

Member

jnwng commented Apr 2, 2014

@heff updated to use the children property for the sake of ie8.

@heff heff merged commit c7e35e5 into videojs:master Apr 3, 2014

1 check passed

default The Travis CI build passed
Details
@@ -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_),

This comment has been minimized.

@heff

heff Apr 3, 2014

Member

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.

@heff

heff Apr 3, 2014

Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment