Renderer demo: Code review, please?

[Colin Clark]

Hi Anastasia,

Nice work on this! I think this is a really good and clear  
illustration of how to use the Renderer in several common scenarios.  
Here are some code review comments, followed by specific responses to  
your questions inline:

In each of your "build subtree" functions, you seem to copy the  
treeChildren variable before adding things to it. I can't quite figure  
out why. Am I missing something, or is it just a bit of cruft left  
over from a refactoring?

In those functions, too, you can probably just return the tree array  
as you build it, rather than giving it a name and immediately  
returning it.

Given that you've nicely factored out each bit of tree generation into  
a separate function, maybe it makes sense to split out the  
stringsTree, too. What do you think?

Just for clarity's sake, it's probably worth organizing demo.render()  
into discrete sections, perhaps like this:

  1. Setup the model (make the applier, register listeners, set up the  
options)
  2. Build the component tree
  3. Bind any event handlers/do setup code

The implementations of buildLocationSubtree() and  
buildWineListSubtree() are pretty much identical, except for the  
various ID names. Indeed, even buildCanapeList() shares the basic  
functionality of setting up the UISelect component. That said, I tried  
to refactor it out and found that it made for a more complicated demo.  
In my mind, this speaks to a couple of things:

  * There should be more high-level functions for generating  
selections + inputs, etc, built into Infusion
  * Managing IDs in a component tree makes parameterization really  
awkward

These, again, are things we'll address in the next big Renderer release.

On 25-Sep-09, at 2:30 PM, Anastasia Cheetham wrote:
> Given that the purpose of the demo is to illustrate some of the  
> 'best practices' for working with the demo, I'd appreciate it if  
> someone could look over the code and check whether or not I'm  
> actually using any of the 'best practices' :-)
> ...
> Regarding cutpoints: what is the 'right' way to use cutpoints? I'm  
> essentially hard-coding them, but that doesn't seem like a 'best  
> practice'.

I think you've used them correctly here. In the next release of the  
Renderer, we will integrate it with the DOM binder so that you can  
always just refer to selector names, abstracting out the mapping  
between IDs and selectors wherever possible.

> I'd also appreciate comments on my use of the ChangeApplier to  
> trigger the display of the changed model - will that complicate  
> things?

I think it makes sense, and clearly illustrates how users can listen  
to automatic changes in the model. It's one line of code, so not  
complicated in my mind.

> Finally: Does anyone know an easy way to pretty-print a JSON object?  
> I'm looking around, but not finding any help.

Pretty printing would make a huge difference here. The library Jacob  
uses for the portal itself might do it, but I'm not sure.

I like seeing the raw model in this demo to see how autobinding works,  
but's pretty overwhelming as-is. Ideally, we might even highlight the  
sections in the model that change based on selections. Too much for  
this release?

Again, nice work!

Colin

---
Colin Clark
Technical Lead, Fluid Project
Adaptive Technology Resource Centre, University of Toronto
http://fluidproject.org