My Collections state

Justin Obara obara.justin at gmail.com
Thu Nov 12 15:02:03 UTC 2009


Hello Sveto,

Sorry I had meant to do this review yesterday, but got side tracked.

It looks like things are coming along and you seem to have been able to pick up and start fairly quickly. 

Please see below for a list comments that I have made.

- Justin


File: engageConfig.json
---------------------------------
line: 8

will probably want to switch to the string templating that we are using, when you start pulling in different content.


File: myCollection.html
-------------------------------
line: 28 - 34

We tend to not use the jQuery(document).ready function in favour of a script block at the end of the body. The jQuery().ready function can cause the screen to flash at times.


line: 10 - 43

By convention we tend to put the link tags above the script tags.



File: myCollection.js
---------------------------

line: 175 - 179

The options are meant to be configurable by an integrator. However, your code here will overwrite any user configured value for styles.listGroup with either "fl-grid" or "fl-list". The first thing you should probably do is to have two default styles instead of one. 

styles: {
    …
    listGroup: "fl-list",
    gridGroup: "fl-grid",
    …
}

This will require some changes to other parts of your code. For example the method for toggling the class. Also you seem to have made a decision that the grid will always be the initial choice. You could make this configurable through the defaults. For example you could add a key in fluid.defualts like defaultView: "grid",  and then have some sort of check in your code before you setup the styles. Another way would be to let this be dependent on the template used. In that case whatever style is given in the markup, will be the default.

line: 99:

Since you aren't returning anything and you are looping over the contents of a jQuery object you could probably have just used the jQuery().each function. It would look something like this:

that.locate("lists").each(function (index) {
    fluid.merge("merge", componentOptions, extractArray(that.options.lists, "listOptions")[index]);
});

docs: http://docs.jquery.com/Core/each

line: 86 & 194

The render and that.reRender functions are almost identical. You should combine these functions and just have the differences switched based on a parameter or a conditional. You could probably do this by checking if the template (the return from your render function) exists.

line: 256

The addClickEvent has the signature (that, template) but only the "that" is used.

line: 235 - 254

Any component that you use inside of your component should be, where possible, used as a subcomponent. 

In this case you would define the reordered in the defaults like this:

imageReorderer: {
    type: "fluid.reorderImages",
    options: {
        selectors: {…},
        styles: {….}
    }
}

Then when you go to initialize it, you would do something like

that.imageReorderer = fluid.initSubComponent(that, "imageReorderer", [that.locate("selectorRepresentingTheContainerForTheReorderer"), fluid.COMPONENT_OPTIONS]);

By doing it this way, you allow an integrator to configure the reorderer component as well.

**General Cleanup**

Any and all selectors and styles used should be declared in the defaults and accessed through that.options. or that.locate

There seams to be left over code that I didn't notice being used, this could probably be removed. (e.g. the useCabinet option)

 I also noticed some syntax errors (e.g. missing ";"), you should run your code through jslint.org. Start by selecting "the good parts" and then modify as necessary. I've attached a screen shot of what that might look like.



On 2009-11-11, at 1:00 PM, Svetoslav Nedkov wrote:

> 
> Hello Justin,
> 
> I've commited the latest for My Collections and expecting your review.
> 
> In the moment what should work is the data extraction from couch db, the rendering of the data as list and grid and the transition between the two.
> 
> 
> Here are the files I've added to the project (relative to my branch):
> 
> fluid-engage-core/components/myCollection/css/myCollection.css
> fluid-engage-core/components/myCollection/html/myCollection.html
> fluid-engage-core/components/myCollection/js/myCollection.js
> 
> /fluid-engage/src/main/webapp/services/myCollection/js/myCollection.js
> 
> I've also modified engageConfig.json and KettleIncludes.json.
> 
> 
> The URL for my branch is:
> 
> https://source.fluidproject.org/svn/scratchpad/myCollection-tryout
> 
> 
> 
> Talk to you soon,
> 
> Svetoslav
> _______________________________________________________
> fluid-work mailing list - fluid-work at fluidproject.org
> To unsubscribe, change settings or access archives,
> see http://fluidproject.org/mailman/listinfo/fluid-work

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://fluidproject.org/pipermail/fluid-work/attachments/20091112/d180e876/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Screen shot 2009-11-12 at 8.51.55 AM.png
Type: image/png
Size: 73320 bytes
Desc: not available
URL: <http://fluidproject.org/pipermail/fluid-work/attachments/20091112/d180e876/attachment.png>


More information about the fluid-work mailing list