Thanks cindyli - this implementation is looking very close. Only some minor
comments:
js/VideoPlayer_intervalEventsConductor.js:
i) The finalInit function at line 63 is redundant, the standard model grade automatically creates an applier
(you will need to add the model grade!)
tests/js/VideoPlayerIntervalEventsConductorTests.js
ii) The unit tests for this component should not use the HTML5 media element (which will probably not run on
IE8 etc.) - this aspect is already taken care of by the integration tests for this component. This test case
should use a synthetic, test-driven driver for the conductor as we discussed.
iii) l87, 110: "success" and "error" are probably misleading names for these test cases since they are not
really testing any failure of the component - rather they are testing the change and non-change of the
interval.
js/VideoPlayer_captionLoader.js
iv) Thanks for the improved commenting and returned public implementation for "convertToMilli". Where do
such timestamps come from? Is it from one of the standard subtitle file formats? If so, this should be
documented, and the conversion function parameterised in the component rather than hardcoded in the impl at
lines 77 etc.
Component is looking pretty good now!
Cheers,
A
On 01/02/2012 09:00, Cindy Qi Li wrote:
@amb26 @colinbdclark : This pull request is ready again for the review. Thanks.
---
Reply to this email directly or view it on GitHub:
https://github.com/fluid-project/videoPlayer/pull/13#issuecomment-3761137
_______________________________________________________
fluid-work mailing list - [email protected]
To unsubscribe, change settings or access archives,
see http://lists.idrc.ocad.ca/mailman/listinfo/fluid-work
