UIOptions code review review - FLUID-4171
Antranig Basman
antranig.basman at colorado.edu
Fri May 13 08:09:21 UTC 2011
On 12/05/2011 13:37, Li, Cindy wrote:
> Hi Antranig,
>
> Thanks for the valuable suggestions. I've been working on them and having a question in regards to
>
>> 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
>
> Can you elaborate on "boundedRangeOption" component? Is it meant to be another sub-component of textfieldSlider? I'm also wondering how to implement "a) construction of component tree" in a separate component outside of "controls" that is the actual renderer component producing the component tree.
>
> I was talking with Yura today about your suggestion and how to make "textfieldSlider" a better self-contained component. He suggested to make "textfieldSlider" a renderer component itself by taking out the markup (https://github.com/cindyli/infusion/blob/FLUID-4171/src/webapp/components/uiOptions/html/UIOptions.html#L16-17) from the uiOptions template into its own little template, in which way the textfieldSlider rendering is encapsulated. This seems a good approach. My only worry is that with the current way that textfieldSlider markup is a part of the whole, it's easier for users to attach "id" or "name" attributes with the<input> field of the textfieldSlider, as what it is doing now in UIOptions template. Once the textfieldSlider template is separated out, users will have to use js to glue them on. Any ideas?
>
> Thanks.
>
> Cindy
"boundedRangeOption" should be some form of "companion component" to textfieldSlider... textfieldSlider is
ONE of the UIs it may be rendered with, but it could take any number of forms. textfieldSlider is nicely
self-contained at present and shouldn't be intruded on. bRO will share the model and applier with the slider
but nothing else (or it may actually share the model and applier with UIOptions itself, and collaborate with
the slider via change events - but in either case it is just the model state which is the point of shared
awareness) - these will be delivered by IoC in the case they are collaborating. However, some code which is
currently part of the implementation of textfieldSlider - for example, the range validation function, will
be migrated out to be "shared" or managed with bRO, whatever form it takes.
In fact the relationship will probably be the other way round - the textfieldSlider will be attached as a
"decorator" subcomponent to the bRO which will be a top-level subcomponent of UIOptions. The main aim of bRO
is to ensure that the act of configuring UIOptions to add or remove one of the options like textSize,
lineSpacing, etc. consists ONLY of adding or removing a new IoC configured subcomponent to UIOptions
configuration. It should not be necessary to touch the UIOptions code at all.
Making textfieldSlider a renderer component is a good general idea but isn't too urgent compared with this
other work. It's a higher priority to make sure that we have a simple story for people who want to add new
option values to UIOptions.
In the refactored design, UIOptions will have a few categories of children, reflecting the different
semantics of the different value types... it looks like we only have two types of values right now, boolean
values, and "bounded ranges" but it's possible we will eventually have more.
UIOptions will treat these child components generically, but it will need to interact with their lifecycles
explicitly at various points - for example
i) to collect whatever selectors they contribute, at startup
ii) to collect whatever component tree and other rendering material they contribute, during rendering
iii) to collect together (or scatter) the separate pieces of model state managed by the subcomponent (this
is done right now in a kind of "half-generic" way, for example, in the "createSliderNode" function). This
function will go into the "produceTree" method of the new bRO (which might indeed in the end defer to a
produceTree method of a future renderised textfieldSlider). Since our changeApplier collaboration system is
still not well-developed in this release, the produceTree will need to be explicitly parameterised by an
option to the bRO suppying its "path offset" in its model, which it will share with overall UIOptions. JURA
should be able to help out with this issue since "model collaboration" is an issue we face in CSpace a lot.
iv) the last leg in the UIEnhancer - to actually ENACT whatever presentation change is represented by a
particular bit of model state
Other types of "collection" should just happen naturally - for example, the min/max values of textSize which
are currently dumped randomly at top level in uiOptions will simply form part of the configuration of the bRO.
In my review mail I identified 4 scattered bits of configuration, although the line numbers have now
changed, relating to these functions (213, 222-225, 352 and 472-475) which all need to be centralised
together into the same block of configurable material.
Hopefully this helps in making the purpose of this component clearer... it may well be too much work to
completely for 1.4 but we should try to make start on the basics. There is a lot of leeway for different
choices in implementation details, but the basic goal is clear - "Turn all of the code binding points
between UIOptions, and, say, the 'text size' configuration option, into pure configuration material which
describes an IoC configured subcomponent of UIOptions, which has a type associated with the fact that its
model value type is a bounded range". We will of course do the same for all the boolean-valued options too,
but that will be a much simpler task - almost trivial once we have a stable design for the range values :)
Cheers,
Antranig.
More information about the fluid-work
mailing list