Quick review of Video Player FLUID-3416

[Colin Clark]

Hi Charly,

Sorry for the delay in getting back to you about the work you've done on the Video Player's FLUID-3416:

  http://issues.fluidproject.org/browse/FLUID-3416

The fork with your changes is here:

  https://github.com/lahabana/videoPlayer

First off, nice work! It's working and looking really good. I even tried it in IE and it still degrades gracefully to Flash.

Here are some code review suggestions:

 * I think your formatTime() function should probably be publicly visible so that you can write some unit tests for it. It provides very useful behaviour, and I can imagine that we'll want to use it in lots of places. The Video Player in general tends to hide too much useful functionality--you'll notice that more recently we've started to try to make useful code public. That lets us and our users take advantage of the functionality in multiple spots, override it if necessary, and allows us to more easily test the code in isolation. So, I'd suggest you name the function fluid.videoPlayer.formatTime().

* I notice that the code at lines 412-416 and 426-430 is pretty much identical. The only difference is the class name injected into the markup for the time display element. I'd suggest we factor that logic out into a more usable form. When I was working on the Uploader's Image Gallery demo, I created a small component called the "simpleRenderer." It should do everything you need. Perhaps you'd like to take that code and prepare it for inclusion in the framework? It shouldn't be much work at all, and I'm happy to help you with the process. Here's the code I'm referring to:

  https://github.com/fluid-project/image-gallery/blob/master/js/image-gallery.js#L188-206

* I've found one minor bug: for the first ten seconds, the time is missing a padding zero. So, it displays "0:0" instead of "0:00" That should be pretty easy to fix.

* Try running your code through JSLint to catch a few minor stylistic and syntax errors:

http://swarm.fluidproject.org/jslint/

Really, really nice first feature. Congratulations! If you fix up these issues, I'd be happy to push this to the project repo. And I promise I won't take so long to get back to you next time. :)

Keep up the good work,

Colin

---
Colin Clark
Technical Lead, Fluid Project
http://fluidproject.org