yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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/

-- 
https://code.launchpad.net/~bcsaller/juju-gui/framework-docs/+merge/136578
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bcsaller/juju-gui/framework-docs into lp:juju-gui.