Review for UIOptions FLUID-4235

Antranig Basman antranig.basman at
Thu May 26 06:40:38 UTC 2011

Hi there, I am casting an eye over this branch as requested -
Commit is at
UIOptions.js at
UIEnhancer.js at

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 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.

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

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

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 "" and only resolved as "cookieStore" via a 
demands block. Right now, someone advising this implementation would have to issue a demands block against 

More information about the fluid-work mailing list