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
_______________________________________________________
fluid-work mailing list - [email protected]
To unsubscribe, change settings or access archives,
see http://fluidproject.org/mailman/listinfo/fluid-work
