Issue 6858085: D3 Components Documentation
|
Created:
12 years, 4 months ago by bcsaller Modified:
12 years, 4 months ago Reviewers:
mp+136578 Visibility:
Public. |
DescriptionD3 Components Documentation
This make an effort to provide a high level modules writer guide. As a
concession it refers the reader to portions of the YUI event and D3 data
binding docs.
https://code.launchpad.net/~bcsaller/juju-gui/framework-docs/+merge/136578
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 37
Patch Set 2 : D3 Components Documentation #Patch Set 3 : D3 Components Documentation #
MessagesTotal messages: 9
Please take a look.
Sign in to reply to this message.
Thanks for these docs, they're much useful. I made quite a few small adjustments to the text flow and markup, so instead of wasting your time and mine by making lots of small comments inline, I just created a new branch and made all changes there. I did not reflow paragraphs even when lines got too long, to avoid obscuring the changes in the diff. Note that some additions and deletions of blank lines are needed to make the document compile to HTML correctly. I also added the document to the index.rst file, so that it appears in the generated output. I'm sorry for the extensive rework: I do believe it significantly improves overall readability, however. Please find the changes at lp:~teknico/juju-gui/framework-docs .
Sign in to reply to this message.
On 2012/11/28 12:33:03, teknico wrote: > Thanks for these docs, they're much useful. > > I made quite a few small adjustments to the text flow and markup, so instead of > wasting your time and mine by making lots of small comments inline, I just > created a new branch and made all changes there. I did not reflow paragraphs > even when lines got too long, to avoid obscuring the changes in the diff. > > Note that some additions and deletions of blank lines are needed to make the > document compile to HTML correctly. > > I also added the document to the index.rst file, so that it appears in the > generated output. > > I'm sorry for the extensive rework: I do believe it significantly improves > overall readability, however. > > Please find the changes at lp:~teknico/juju-gui/framework-docs . Thank you! I merged your changes to this branch and will re-propose if there is another reviewer.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... docs/d3-component-framework.rst:111: :: This is the shorthand:: https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst... 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... 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... docs/d3-component-framework.rst:122: :: This is the full form:: https://codereview.appspot.com/6858085/diff/1/docs/d3-component-framework.rst... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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... 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.
Sign in to reply to this message.
To be clear, I am fine with you landing this with the document changes that I have suggested (or similar ones, after reconciling my review and the other). I do not need for the framework changes I mentioned to be made in this branch. If you agree with those suggestions, though, please make bugs and cards for them, or ask me to. I would be happy to.
Sign in to reply to this message.
On 2012/11/28 14:12:29, gary.poster wrote: > To be clear, I am fine with you landing this with the document changes that I > have suggested (or similar ones, after reconciling my review and the other). I > do not need for the framework changes I mentioned to be made in this branch. If > you agree with those suggestions, though, please make bugs and cards for them, > or ask me to. I would be happy to. Thanks for the great review. I've gone and updated the doc in a way that I hope is reflective of where we are headed while still being immediately useful. I'm going to go ahead and submit it next.
Sign in to reply to this message.
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... docs/d3-component-framework.rst:67: section on events below). On 2012/11/28 14:10:29, gary.poster wrote: > 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. These are good ideas. There are some special cases where render might be called more than once (detach/re-attach to DOM for example) which I've added to the doc. Its also nice for modules to be assured that things like the svg element will be present on the container (via its renderOnce) so that when each modules render is called they can draw reliably without lots of extra checks. I've expanded this section to indicate this (I hope clearly).
Sign in to reply to this message.
*** Submitted: D3 Components Documentation This make an effort to provide a high level modules writer guide. As a concession it refers the reader to portions of the YUI event and D3 data binding docs. R=teknico, benjamin.saller, gary.poster CC= https://codereview.appspot.com/6858085
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
