Hi Ben. This is really a great improvement. Thank you. I have a number of suggestions, and I noticed that Nicola has a branch of changes. I hope it is not too hard to reconcile our two reviews. I did have a few observations about the framework itself. I would be happy to discuss them if you would like. Thanks again. Gary https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst File docs/d3-component-framework.rst (right): https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode21 docs/d3-component-framework.rst:21: models one concern of the resultant Scene. This is a reasonable basic definition of a module. It could be expanded: what do you mean by a "concern"? What's a guideline for dividing up these concerns? Beyond that, some of your other terms could use definitions. You start to define a "component" in line 35, but what is the relationship between d3 and the "component"? Is the component expected to correspond with a scene? What is the component a component of? A definition of scene (which you capitalize here for a reason that isn't clear to me) would be helpful as well. Perhaps the definition of component should come before that of module because this is a "component" system. Perhaps also these definitions deserve their own section, but that's just one way of many to handle it. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode37 docs/d3-component-framework.rst:37: added options can be passed that will be made available in the modules runtime. Modules' run times or module's run time. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode38 docs/d3-component-framework.rst:38: Modules added by via the `addModule` method automatically have a reference to "Added by the" or "added via the" https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode40 docs/d3-component-framework.rst:40: the addModule call in an Attribute call options which will default to an empty …the addModule call result in an attribute called 'options,' which… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode47 docs/d3-component-framework.rst:47: This example would create a new component and using the MyModule constructor …and then, using the MyModule constructor, create… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode56 docs/d3-component-framework.rst:56: Components then support rendering, drawing the scene described by any data the I found this sentence hard to parse. "…which draws…" maybe? https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode64 docs/d3-component-framework.rst:64: information. Once update has been called on on the modules each modules …on the modules, each module's… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode67 docs/d3-component-framework.rst:67: section on events below). In terms of the framework itself, I wonder if the first render should not call the normal render behavior. In other words, it should renderOnce and then update and then stop. In fact, if update is supposed to be about incremental updates, and renderOnce is about preparing for an update, then the render method seems superfluous. The pattern that seems to be falling out to me is this: when you call render on a component, it calls render on a module, and then it calls update on a module. You never have to call render on the module again. Effectively, renderOnce become render, and the current render disappears. This seems is simpler and sufficient. If it is not sufficient, then my understanding is insufficient, and we might need some more explanation here of some sort. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode72 docs/d3-component-framework.rst:72: module and when removeModule is called it will properly clean up event …module, and when… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode76 docs/d3-component-framework.rst:76: As a final step if the module has a `componentBound` callback it will be …step, if the… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode82 docs/d3-component-framework.rst:82: Events You're actually talking about the event handlers, not events. This change should be propagated throughout the section below, I think. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode85 docs/d3-component-framework.rst:85: The heart of the component system events can be defined in a number of ways and The heart of the component system – event handlers – can be defined in a number of ways. Understanding how to take advantage… Or Event handlers are the heart of the component system. They can be defined in a number of ways. Understanding how to take advantage… Or even more simply, you can leave out the mention of definitions here. You talk about that in the next paragraph. I would simply say the following: Event handlers are the heart of the component system. Understanding how to take advantage… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode86 docs/d3-component-framework.rst:86: understand how to be take advantage of the binding features will greatly aid in … how to take… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode87 docs/d3-component-framework.rst:87: producing a system which allows for clean, clear, well separated concerns and …a system that allows for clean, clear, well separated concerns, and which in turn… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode92 docs/d3-component-framework.rst:92: module declaration but a module writer must understand them all to properly use … module declaration, but a module writer… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode95 docs/d3-component-framework.rst:95: When modules are added three sets of declarative events are bound. This is done When the modules are added, three sets of declarative event handlers are bound. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode105 docs/d3-component-framework.rst:105: delegation. This has advantages in that it scales well and these events can be Scene event handlers use YUI style event delegation. This scales well because these handlers can be bound once to the top level container object of a component, and will work… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode107 docs/d3-component-framework.rst:107: an DOM element matching its selector. Scene events are defined in one of two "…any DOM elements…" …event handlers are defined in one of two ways… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode108 docs/d3-component-framework.rst:108: ways, the first is a shorthand, the later is the more complete definition …ways: the first is a shorthand, and the second is the more complete definition… Or alternatively …ways: the former is a shorthand, and the latter is the more complete definition… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode111 docs/d3-component-framework.rst:111: :: This is the shorthand:: https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode117 docs/d3-component-framework.rst:117: scene: {selector: function() {...}} I suggest deleting lines 114 through 120, and explaining this anonymous function option *after* you present the two primary spellings. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode119 docs/d3-component-framework.rst:119: Though the string name of a callback is preferred as this makes the whole Delete "though": The string name of a callback is preferred, as this makes the whole set of events more easily readable. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode122 docs/d3-component-framework.rst:122: :: This is the full form:: https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode127 docs/d3-component-framework.rst:127: Here, you could explain the anonymous function option. "In both spellings, you can replace the string name of a callback with an anonymous function. The name is preferred because it is more easily readable." https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode128 docs/d3-component-framework.rst:128: Regardless of form selector is a CSS selector, typically either a `.class` or In both event handler definitions, the "selector" is a CSS selector; typically either a… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode129 docs/d3-component-framework.rst:129: an `#id` though pseudo-selectors work as well. With scene events these `#id`, though… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode137 docs/d3-component-framework.rst:137: clicked invoke the 'personClick' handler. Handlers all have a common signature. …clicked, invoke… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode151 docs/d3-component-framework.rst:151: In the near future scene events will support an additional context attribute in Scene event handler registrations, right? https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode152 docs/d3-component-framework.rst:152: their handler definition which can either be `component` or `module` and will …handler definition, which can… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode160 docs/d3-component-framework.rst:160: similar to scene events D3 events are bound after the modules render method is …similar to scene events, D3 events are bound after the module's render… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode161 docs/d3-component-framework.rst:161: triggered as DOM elements must be present to be bound. There are very few cases …triggered, because DOM elements… There are very few situations in which this style of event binding is preferred over normal scene events, but there are legitimate uses. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode163 docs/d3-component-framework.rst:163: are legitimate uses. If the event is a D3 synthetic event such as zoom or drag …such as zoom or drag, using D3 event bindings… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode166 docs/d3-component-framework.rst:166: certain mouse events are dealt with more easily using D3 events as D3 uses a …more easily using D3 events, because D3 uses a well documented and clean system of X, Y position coordinates. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode182 docs/d3-component-framework.rst:182: The final type of event is called 'yui' events. This classification doesn't Would it be more helpful to the user of the system to categorize and name this collection of event handlers as "custom" handlers? "YUI" does not seem helpfully descriptive, since we also use YUI events for scene handlers. https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode203 docs/d3-component-framework.rst:203: In this example another module might fire a 'cancelAction' event, our module In this example, another module might fire a "cancelAction" event. Our module wants to respond to this… https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst#newcode214 docs/d3-component-framework.rst:214: While the frameworks tests show off these features here is a complete example of Here is a complete example of a module, with some description. The tests for this framework also can be used to learn about the capabilities and expected usage of the system. https://codereview.appspot.com/6858085/