search

mickey / videojs-ga

Google Analytics plugin for video.js
MIT License
138 stars 109 forks source link

player.techName is undefined when using minified video.js #4

Closed dominic-p closed 10 years ago

dominic-p commented 10 years ago

I noticed that the script uses this.techName, but I don't think that property is exported for use in the minified version. I get "undefined" in my GA reports.

Maybe some logic could be implemented that checks for player.Html5 and player.Flash instead of the techName property. You can see the list of exported properties in the exports.js file in the main video.js repo.

mickey commented 10 years ago

Hey @dominic-p, you're right the property wasn't exported. To fix, I used the .vjs-tech dom element that has an id of either video_html5_api or video_flash_api.

dominic-p commented 10 years ago

Hey @mickey, that's an interesting approach. I'm concerned that using getElementsByClassName will break IE8. Also, would this approach work with alternative techs like YouTube?

I opened up an issue on the main video.js repo to see if a better way can be found.

mickey commented 10 years ago

Hey @dominic-p Thanks a lot for digging into this. I just checked MDN and it's true getElementsByClassName doesn't work on IE8 :shit:. I have no idea if it would work with other techs, are you thinking of videojs-youtube or something else?

Thanks a lot for creating the issue on the video.js repo. I'm thinking about removing this feature until the video.js maintainers do something about it. What do you think?

dominic-p commented 10 years ago

No problem. Yeah, I was thinking about videojs-youtube or possibly some of the other external techs.

I agree, it would probably be best just to remove this until we have a better way of accessing this information. Although, from what I'm seeing on the issue I created, the tide definitely seems to be against making this information accessible in video.js. We'll see how it pans out though.

mickey commented 10 years ago

Never tried it but I don't like the hack too. I commented on the issue with a possible solution, it might be wrong (it's past midnight in here ^^). I'll remove the feature this week until we have a better way.

Thanks again for your feedback and interest.

mickey commented 10 years ago

I removed the feature until video.js provide a public function to get the tech. Thanks again for the feedback.