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 ________________________________________ From: [email protected] [[email protected]] On Behalf Of Antranig Basman [[email protected]] Sent: 11 May 2011 01:09 To: Fluid Work Subject: UIOptions code review review - FLUID-4171 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 _______________________________________________________ fluid-work mailing list - [email protected] To unsubscribe, change settings or access archives, see http://fluidproject.org/mailman/listinfo/fluid-work
