Response to your last code review and FLUID-3116, FLUID-3117

[Laurel A. Williams]

Hi Justin et al.

Thanks for your compliments and your prompt response to my request for 
review. I addressed your first two email points in the two jira's listed 
in the subject line and have checked in those changes.


I think you have a good point about the test data in builder.js but we 
should discuss it further - Michelle and I also chatted about this data 
structure, so it is worth bringing this question to her attention as well.

As I'm sure you recall, the aim of the customBuild component is to to 
populate the custom build page from the live data from the 
build.properties file. For the demo that we've created, we provide two 
ways to populate the page. The first technique provides the data via a 
hard coded data structure 
fluid.customBuild.demo.completeFluidInfusionData (which I guess is only 
complete today and will likely change considerably over time). The 
second technique provides the data live via the build properties file.

You can see code to use the two different techniques in CustomBuild.html 
and will recall that we have been commenting out the specific 
builderInit lines depending on which technique we want to use - the 
stand alone data structure works well for testing without dealing with 
any of the php code.

<snip>
//to use test data use the following
           builderInit("#customBuild", 
fluid.customBuild.demo.completeFluidInfusionData);
          
//to use data from infusion json files use the following
//           builderInit("#customBuild", fluid.customBuild.dependencies);
</snip>

As I was working with the tests, I noted that we reproduced the exact 
same data in the tests. I refactored to use only one copy of the data, 
but perhaps it was a case of "premature optimization".

Note though, since this is a piece of demo code, the 
fluid.customBuild.demo.completeFluidInfusionData data does not need to 
change - it could stay the same even as infusion expands, since the data 
for the 'live customBuilder' will always come from the build.properties 
file and not from this data structure. However, I'm thinking that people 
will be tempted to change the data as the real infusion expands, in 
which case they will have to change the tests to reflect those 
changes...and some of the hard coding will be a pain to change.

I did try to address this somewhat by providing some constants for the 
indexes of the specific modules in the data which you wrote tests 
around. In customBuild-tests.js you can find:

        var INLINE_EDIT_INDEX = 7;
        var PAGER_INDEX = 8;
        var PROGRESS_INDEX = 9;
        var REORDERER_INDEX = 10;
        var UIOPTIONS_INDEX = 12;

So, I think that outlines all of the details around this data 
structure...what do you think...should it simply be duplicated in both 
the tests and the demo to prevent people from breaking the tests 
inadvertently? I'm not convinced, but honestly don't think it is 
critical one way or the other.

Laurel

Justin wrote:
> Hello,
>
> I've looked over some of your commits, please see my comments below.
>
> customBuild.js
> ============
>
>     * the code for checkElementArray and unCheckElementArray seem to
>       be very similar. You could probably abstract that out into a
>       general function that is called with different parameters
>     * This is one of those things I thought about a while ago, but
>       forgot about. The onModelChange event should probably be renamed
>       to afterModelChanged or something like that.
>
>
> customBuild-tests.js
> ================
>
>     * What is the strategy going forward for
>       fluid.customBuild.demo.completeFluidInfusionData. It probably
>       shouldn't pull data in from builder.js but have some test data
>       within the tests directory, or will this be pulling in some live
>       data in the future.
>     * Depending on the above, you may want to look into not having the
>       module indexes hardcoded for your tests.
>
>
> It's looking good. Hope the feedback helps.
>
> - Justin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.idrc.ocad.ca/pipermail/fluid-work/attachments/20090820/1596304b/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: laurel_williams.vcf
Type: text/x-vcard
Size: 269 bytes
Desc: not available
URL: <https://lists.idrc.ocad.ca/pipermail/fluid-work/attachments/20090820/1596304b/attachment.vcf>