Review for UIOptions FLUID-4235

Obara, Justin jobara at ocad.ca
Thu May 26 17:27:09 UTC 2011


Hi Michelle,

I think adding the new jiras to bug parade makes sense.

Thanks
Justin
On 2011-05-26, at 1:14 PM, D'Souza, Michelle wrote:

Thanks for the review Cindy and Antranig.

If it's ok with you, I'm thinking of addressing these under new JIRA issues instead of as part of 4235. I think this work will cause havoc with the ongoing work Cindy, Heidi, Justin and James have been doing and I'd rather do it after I've created the merge branch we talked about in dev meeting.

Justin, is it ok if I add these JIRA issues to bug parade since they came out of code review?

I'm now going to create the merge JIRA and branch as we discussed in dev meeting and merge all the ongoing work for UI Options. Then I'll continue with the refactoring you've suggested. I've put more comments inline below.

Michelle


On 2011-05-26, at 2:40 AM, Antranig Basman wrote:


For UIOptions.js:
Yes, I support cindyli's comment that dependency design has failed here - there should not be a reference to something called "UIEnhancer" from UIOptions. The enhancer should only appear at the level where there is a required shared dependence, for example, in fluid.uiOptions.preview. [see suggestion below]

One resolution would be to reorganise the lines 242-245 to be delivered by a demands block, that only acted when a {uiEnhancer} was in scope. It is still problematic that so many defaults are defined within UIEnhancer itself - for example, the settingsStore and the defaultSiteSettings etc. I would move these out into a 3rd, largely dependence-free component called something like UIOptionsStore - they would be injected into both UIEnhancer and UIOptions from some shared location. See UIEnhancer.js:264 comment below


I created this JIRA:

FLUID-4265: Create a  UIOptionsStore which encapsulates defaultSiteSettings and settingsStore


I would actually refactor this area quite significantly - uiOptions.preview should not be a subcomponent of uiOptions but actually a container/sibling of it. *OR* the thing currently called uiOptions should actually be renamed uiOptionsDialog - and this is the thing which has the link JUST to settingsStore and nothing else. Then the name "uiOptions" can be used for the "orchestrating component" which contains a preview, a uiOptionsDialog, a settingsStore and a uiEnhancer as direct children. Almost all the members which are currently on uiOptions (basically all the "controls") would go onto uiOptionsDialog.


This is the JIRA I created:

FLUID-4266: Refactor UIOptions to be an orchestrating component


lines 90 can be replaced by the all-new "renderOnInit" flag :P


Antranig, doesn't the component need to be a renderer component in order to use the renderOnInit flag?


I believe that line 427, that.refreshView() is redundant? I think it is gone from cindyli's branch anyway...

I'm leaving this as is since it will be fixed in the UIOptions merge branch.



For UIEnhancer.js:
The sequence at line 330 is somewhat similar to the way "fluid.uploader" forwards its options and is something we need to produce a proper framework pattern for.

line 264: This should just be specified as "fluid.uiOptions.store" and only resolved as "cookieStore" via a demands block. Right now, someone advising this implementation would have to issue a demands block against "fluid.uiEnhancer.cookieStore"

This will be address in FLUID-4265



------------------------------------------------------
Michelle D'Souza
Inclusive Software Developer Researcher
Inclusive Design Research Centre


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.idrc.ocad.ca/pipermail/fluid-work/attachments/20110526/a899ddf8/attachment.htm>


More information about the fluid-work mailing list