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.

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

Export createItems call to enable plugins to hook into process #654

Closed

Conversation

dominic-p
Copy link
Contributor

The pull request is more of a request for feedback. This seems like the path of least resistance to enable plugins to add menu buttons (see #648), but it's entirely possible that I've missed something obvious (sorry if that's the case).

@heff
Copy link
Member

heff commented Jul 29, 2013

Cool, good idea. I would instead export that here:
https://github.com/videojs/video.js/blob/v4.1.0/src/js/exports.js#L92

goog.exportProperty(vjs.MenuButton.prototype, 'createItems', vjs.MenuButton.prototype.createItems);

And then use a string key anywhere createItems is called

this.items = this['createItems']();

If you want to update that and make sure it works, I'll pull it in.

@dominic-p
Copy link
Contributor Author

Ok, the changes have been made. My fork got a little messed up, so I had to get rid of the original commit and start over. I've tested it in my environment, and it seems to be working as expected. Let me know if anything looks amiss to you.

@heff
Copy link
Member

heff commented Jul 29, 2013

Ah, you actually don't need the last three string key updates, vjs.MenuButton.prototype['createItems'] and both in tracks.js. In fact that will probably break those functions. Sorry, that was totally my fault.

@dominic-p
Copy link
Contributor Author

Ok, I rolled back the unnecessary conversions. Not your fault at all. I'm still trying to wrap my head around Closure's advanced compilation.

@heff
Copy link
Member

heff commented Jul 29, 2013

Ok wow, this is frustrating. It looks like those other items need to be exported too in order to work. Instead though, add these lines after the first exportProperty you did in exports.js.

goog.exportProperty(vjs.TextTrackButton.prototype, 'createItems', vjs.TextTrackButton.prototype.createItems);
goog.exportProperty(vjs.ChaptersButton.prototype, 'createItems', vjs.ChaptersButton.prototype.createItems);

Sorry for the runaround. Closure compiler can be frustrating, but it makes things so dang small. I actually hoping to dig into a new minifier project called esmangle in the future.

@dominic-p
Copy link
Contributor Author

Alright, I think that does it. Sorry about the stupid extra file that showed in the last set of commits. I don't think it should effect anything, but let me know if you would like me to wipe all of these commits clean and push a single commit with everything.

@heff
Copy link
Member

heff commented Jul 30, 2013

Awesome, thanks! I can clean up the commits with Pulley.

@heff heff closed this in 15fab6d Jul 30, 2013
@dominic-p dominic-p deleted the feature/export-create-items branch July 30, 2013 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants