Hi there - this is primarily for Cindy, Michelle and Justin, who are currently collaborating on preparing
UIOptions for the 1.4 release at the end of the month. Justin has given the pull request 38 one round of
comments at https://github.com/fluid-project/infusion/pull/38/commits - I think all of the comments which
are attached to individual are basically sound, and carrying the component in the right direction. I wanted
to comment more globally on the component and its strategy since there are a few areas in which it is still
some way from the form we would want.
I will be commenting on the FLUID-4171 branch at revision 5f15d2 - which can be
browsed at
https://github.com/cindyli/infusion/blob/5f15d26cd7710b080ba2a3fff312471be6ea4549/src/webapp/components/uiOptions/js/UIOptions.js
The first dodgy area is in the finalInit function lines 69-73 -
var sliderOptions = that.options.sliderOptions;
sliderOptions.value = that.model.value;
sliderOptions.min = that.model.min;
sliderOptions.max = that.model.max;
"State-shuffling" code of this kind is always suspicious - whenever you see long chains of assignments be
sure that the framework can be doing work for you. In general the textFieldSlider should be making more use
of the changeApplier and framework facilities for dealing with models.
i) These 4 lines should just be replaced by a call to fluid.merge or
jQuery.extend - such as
var sliderOptions = $.extend(true, {}, that.options.sliderOptions, that.model);
If necessary, the specific layout which is of interest being copied can be noted in a comment, in case
there is a worry about the looseness of the contract of aligning the model layout of the component with the
options accepted by jQuery UI slider.
ii) The event "modelChanged" on the textfieldSlider should be removed, and all the collaborating code should
just make use of the events fired from the changeApplier instead (which also happens to be named "modelChanged")
iii) The code packaged in textfield.finalInit lines 103-115 should be broken out into a free function which
performs validation for the model state of the slider. The best way to deliver this would be as a "guard"
for the changeApplier - some docs are at http://wiki.fluidproject.org/display/fluid/ChangeApplier+API and
also the relevant behaviour may be seen in the test cases for the changeApplier in DataBindingTests.js lines
173-208.
Moving on to UIOptions itself - its handling of model state, especially from interacting with UIEnhancer, is
a little odd. In general, this component needs to be completely generic, and should be free of bits of
configuration freely floating at top level.
iv) So, the treatment of "textSize" and "lineSpacing" at lines 222-228 is a bit odd. This state should
instead form part of the configuration of subcomponents of uiOptions which are nested within "controls". The
"current state" of textSize as well as its bounds should be factored into a self-contained component with
well-defined model state. Perhaps something like a "boundedRangeOption" component which in addition manages
a) construction of component tree
b) selector binding to markup in the controls
c) bounds for the range, and
d) model state
So, in the current design, configuration relating to "textSize" is scattered across the whole component. The
fragments are held at lines 213, 222-225, 352 and 472-475 - these should all be held in one place in one
subcomponent.
Always ask yourself - if someone wants to come and modify my component, for example, by creating a new kind
of option - how many places will they need to touch my code? The answer right now is that they need to edit
the physical implementation of UIOptions itself in 4 places. In the ideal design they would not need to edit
the implementation at all, but simply supply different IoC configuration to UIOptions - by adding a new
subcomponent. Clearly the same goes for lineSpacing, which will become a component of the same type, and all
the boolean-valued options, etc.
v) This refactoring will extend out into the UIEnhancer as well, since these "view setting subcomponents"
will to some extent be autonomous ("addStyles", "styleLinks" etc. are all still hardbaked like last month's
bread). A big oddity with our current design which I noticed when working with Clayton is that the
"defaultSiteSettings" record is held in an odd place - in UIEnhancer. It really belongs with the component
holding the state - either UIOptions or in a newly created subcomponent of it.
vi) As I think we had already planned, UIEnhancer also needs to be converted over to IoC - for example, the
ToCSetting logic needs to be moved out into an IoC-configured subcomponent, and just like UIOptions itself,
UIEnhancer needs to be freed of all non-generic configuration appearing at top level. Rather than using
jQuery.data, this impl should instead use the static environment to store the enhancer.
vii) Should we get through all of this, we can talk about the non-standard construction workflow of
UIEnhancer which I think came up today. Certainly it is unusual in that it accepts a document as first
argument rather than a container. This would best be dealt with by moving out responsibility for indirecting
on the document etc. to a supercomponent of UIEnhancer and returning UIEnhancer to be a standard view
component, accepting as its first argument the value which currently ends up manually on that.container on
line 225.
viii) In the "modern style", throughout both of these files, closure-private utility functions should be
replaced by publically namespaced free functions.
Good luck - we should talk over these changes some more at the dev meeting.
_______________________________________________________
fluid-work mailing list - [email protected]
To unsubscribe, change settings or access archives,
see http://fluidproject.org/mailman/listinfo/fluid-work
