New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Y.Tree: An awesome generic tree data structure #429

Merged
merged 11 commits into from Feb 15, 2013

Conversation

Projects
None yet
6 participants
@rgrove
Contributor

rgrove commented Feb 1, 2013

Y.Tree

This is an import of the SmugMug Tree gallery module, a generic tree data structure that provides a fast and friendly API for working with hierarchical data (such as the items in a Menu, TreeView, or even a DOM node).

What it does

A tree has a root node, which may contain any number of child nodes, which may themselves contain child nodes, ad infinitum.

Child nodes (Y.Tree.Node) are lightweight function instances which delegate to the tree for all significant functionality, so trees remain performant and memory-efficient even with thousands and thousands of nodes.

Don't believe me? Here's a stress test that demonstrates the SmugMug TreeView gallery module (which uses Y.Tree) loading and rendering 12,780 nodes in about 100ms: http://jsbin.com/udayaz/54

The Y.Tree class doesn't expose any UI, but the following gallery modules are examples of components that extend Y.Tree and provide a UI:

Why YUI needs it

Tree structures are inherently useful for representing many types of data that web developers work with frequently. YUI currently has components such as Model and ModelList that are good for working with flat lists, but these components are awkward to use and perform poorly when shoehorned into representing hierarchical data.

Some of the most obvious potential use cases for a tree structure would be a treeview widget or a hierarchical menu. Here are a few other possible use cases to spur your imagination:

  • A future version of Y.NodeList could adopt Y.Tree to represent a hierarchy of DOM nodes.
  • Y.App could adopt Y.Tree to represent a hierarchy of views.
  • A parser might use Y.Tree to represent a parse tree.

Act now and get these extensions for free!

This pull request includes several Y.Tree extensions that can be used to mix additional functionality into a tree:

  • Y.Tree.Labelable: Adds the concept of text or HTML "labels" to nodes, like you might see in a treeview or menu.
  • Y.Tree.Openable: Adds the concept of openable/closeable/toggleable nodes to a Tree, along with related events and methods.
  • Y.Tree.Selectable: Adds the concept of selection state to a Tree, along with related events and methods. In single-select mode (the default) one node at a time may be designated as selected. In the optional multi-select node, multiple nodes may be designated as selected.

Also included is a plugin, Y.Plugin.Tree.Lazy, which can be plugged into any Tree that mixes in the Y.Tree.Openable extension. This plugin adds a convenient API for lazy-loading children of a node the first time that node is opened.

Other stuff

Y.Tree and friends have full test coverage and detailed API docs. There currently is not a user guide or examples, but I hereby promise to write those if there's a consensus that this PR should be accepted.

rgrove added some commits Feb 1, 2013

Initial commit of Y.Tree and friends.
Imported from the SmugMug Tree gallery module, which has a more
extensive commit history:

https://github.com/smugmug/yui-gallery/tree/master/src/sm-tree
Split labels out into an extension as recommended by @ericf.
Eric pointed out in IRC that labels are a pretty UI-centric metaphor and
probably don't belong in the base Tree and Tree.Node classes, and I agree.

This commit removes the label implementation from those base classes
and moves it into a Y.Tree.Labelable/Y.Tree.Node.Labelable extension.
@triptych

This comment has been minimized.

Show comment
Hide comment
@triptych

triptych Feb 4, 2013

Contributor

I really like this and am rooting for it to get added quickly.

Contributor

triptych commented Feb 4, 2013

I really like this and am rooting for it to get added quickly.

@lsmith

This comment has been minimized.

Show comment
Hide comment
@lsmith

lsmith Feb 4, 2013

Contributor

👍

Contributor

lsmith commented Feb 4, 2013

👍

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Feb 6, 2013

Member

👍

Member

davglass commented Feb 6, 2013

👍

{
"tree": {
"requires": [
"base-build",

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

This dependency could be reduced down to base-base and the Y.Tree constructor could be created using Y.extend().

@ericf

ericf Feb 6, 2013

Member

This dependency could be reduced down to base-base and the Y.Tree constructor could be created using Y.extend().

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

I retract this because a it will be common for people to use Y.Tree with extensions, and they may run into issues when Y.Base.build() being undefined.

@ericf

ericf Feb 6, 2013

Member

I retract this because a it will be common for people to use Y.Tree with extensions, and they may run into issues when Y.Base.build() being undefined.

},
// -- Public Methods -------------------------------------------------------
load: function (node, callback) {

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

Can we make this method signature consistent with Model's sync features?

@ericf

ericf Feb 6, 2013

Member

Can we make this method signature consistent with Model's sync features?

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

The main thing I see is needing a place for options.

@ericf

ericf Feb 6, 2013

Member

The main thing I see is needing a place for options.

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

There's no way to make this method signature consistent with Model#load() unless we remove the node param, which is required here. That doesn't make much sense to me. What's the benefit?

This extension doesn't extend Model, nor does it behave like a Model, so I don't see a strong reason for it to adhere to Model's method signatures.

@rgrove

rgrove Feb 6, 2013

Contributor

There's no way to make this method signature consistent with Model#load() unless we remove the node param, which is required here. That doesn't make much sense to me. What's the benefit?

This extension doesn't extend Model, nor does it behave like a Model, so I don't see a strong reason for it to adhere to Model's method signatures.

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

This extension doesn't extend Model, nor does it behave like a Model, so I don't see a strong reason for it to adhere to Model's method signatures.

Yeah, I'm not suggesting that it does or should. But if the "sync" features had the same signatures, then custom sync layers could be used with both tree nodes and models.

@ericf

ericf Feb 6, 2013

Member

This extension doesn't extend Model, nor does it behave like a Model, so I don't see a strong reason for it to adhere to Model's method signatures.

Yeah, I'm not suggesting that it does or should. But if the "sync" features had the same signatures, then custom sync layers could be used with both tree nodes and models.

This comment has been minimized.

@rgrove

rgrove Feb 7, 2013

Contributor

I gave it some thought, and I'd like to leave this as is. Consistency with the Model#load() signature would provide a minor convenience to a subset of implementers, but would be a weird signature to work with in virtually every other use case. I don't think it's worth it, especially when someone who wants to use a Model sync layer can just do something like:

tree.plug(Y.Plugin.Tree.Lazy, {
    load: function (node, callback) {
        model.load({id: node.id}, callback);
    }
});
@rgrove

rgrove Feb 7, 2013

Contributor

I gave it some thought, and I'd like to leave this as is. Consistency with the Model#load() signature would provide a minor convenience to a subset of implementers, but would be a weird signature to work with in virtually every other use case. I don't think it's worth it, especially when someone who wants to use a Model sync layer can just do something like:

tree.plug(Y.Plugin.Tree.Lazy, {
    load: function (node, callback) {
        model.load({id: node.id}, callback);
    }
});

This comment has been minimized.

@ericf

ericf Feb 7, 2013

Member

I thought about it too, and I agree tree.load({node: node}, fn); sucks.

@ericf

ericf Feb 7, 2013

Member

I thought about it too, and I agree tree.load({node: node}, fn); sucks.

// Make sure we've been plugged into a Tree that mixes in the
// Tree.Openable extension.
if (!this._host.openNode) {
Y.log("Plugin.Tree.Lazy was plugged into a Tree that doesn't mix in the Tree.Openable extension. This probably won't do you much good.", 'warn', 'tree-lazy');

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

If this is the case, why not merge this functionality into Y.Tree.Openable? Is there harm in an "openable" tree also supporting "lazy nodes" by default?

@ericf

ericf Feb 6, 2013

Member

If this is the case, why not merge this functionality into Y.Tree.Openable? Is there harm in an "openable" tree also supporting "lazy nodes" by default?

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

Making all nodes lazy by default adds implementation cost, since it then requires the implementer to define the load() method for loading child nodes. Not everyone wants a lazy tree, and in fact Tree is so fast and Tree.Node is so light that there's not much reason to use a lazy tree unless you're talking about seriously huge numbers of nodes.

@rgrove

rgrove Feb 6, 2013

Contributor

Making all nodes lazy by default adds implementation cost, since it then requires the implementer to define the load() method for loading child nodes. Not everyone wants a lazy tree, and in fact Tree is so fast and Tree.Node is so light that there's not much reason to use a lazy tree unless you're talking about seriously huge numbers of nodes.

@@ -0,0 +1,48 @@
/**

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

Can we merge this class extension definition into the tree-labelable module? I don't see a need for them to be separate modules since they're close friends :)

@ericf

ericf Feb 6, 2013

Member

Can we merge this class extension definition into the tree-labelable module? I don't see a need for them to be separate modules since they're close friends :)

this.tree.unselectNode(this, options);
return this;
}
};

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

Add toggleSelect()?

@ericf

ericf Feb 6, 2013

Member

Add toggleSelect()?

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

Selection isn't something you generally need to toggle. I've never had a case where I've needed that. It's typically something you're explicit about (which is why I gave toggle() to Openable).

@rgrove

rgrove Feb 6, 2013

Contributor

Selection isn't something you generally need to toggle. I've never had a case where I've needed that. It's typically something you're explicit about (which is why I gave toggle() to Openable).

Show outdated Hide outdated src/tree/js/tree.js
},
destructor: function () {
(new Y.EventHandle(this._selectableEvents)).detach();

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

Neat trick!

@ericf

ericf Feb 6, 2013

Member

Neat trick!

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

Luke taught me this.

@rgrove

rgrove Feb 6, 2013

Contributor

Luke taught me this.

Show outdated Hide outdated src/tree/js/tree-node.js
of `tree.rootNode.children`.
@property {Tree.Node[]} children
@readOnly

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

I think this should be an attribute because it's per-instance state.

@ericf

ericf Feb 6, 2013

Member

I think this should be an attribute because it's per-instance state.

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

Disagree.

tree.children is fast, easy to type, easy to read, and maintains consistency with node.children. It would gain nothing from being an attribute, except becoming slower, harder to type, harder to read, and less consistent.

@rgrove

rgrove Feb 6, 2013

Contributor

Disagree.

tree.children is fast, easy to type, easy to read, and maintains consistency with node.children. It would gain nothing from being an attribute, except becoming slower, harder to type, harder to read, and less consistent.

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

I think people will expect a tree, which is Base-based, to have attributes.

Does the config property, nodes, represent all descendants of this tree or just its children?

@ericf

ericf Feb 6, 2013

Member

I think people will expect a tree, which is Base-based, to have attributes.

Does the config property, nodes, represent all descendants of this tree or just its children?

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

nodes is an array of children of the rootNode, but each child may also have children, and so on. So the answer to your question is yes.

@rgrove

rgrove Feb 6, 2013

Contributor

nodes is an array of children of the rootNode, but each child may also have children, and so on. So the answer to your question is yes.

/**
Root node of this Tree.
@property {Tree.Node} rootNode

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

I also think this should be an attribute because it too is per-instance state.

@ericf

ericf Feb 6, 2013

Member

I also think this should be an attribute because it too is per-instance state.

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

Disagree for the same reasons as tree.children.

@rgrove

rgrove Feb 6, 2013

Contributor

Disagree for the same reasons as tree.children.

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

rootNode is passed into the constructor as a config property, is unique per instance, an is not returned from tree.get('rootNode'). This doesn't seem consistent with a Base-based class.

@ericf

ericf Feb 6, 2013

Member

rootNode is passed into the constructor as a config property, is unique per instance, an is not returned from tree.get('rootNode'). This doesn't seem consistent with a Base-based class.

This comment has been minimized.

@rgrove

rgrove Feb 6, 2013

Contributor

An earlier version of Tree exposed attributes for rootNode and children that simply acted as getters for the properties for the sake of people who enjoy using abstractions for things that don't need abstractions.

But then I thought that maybe those people should learn not to use unnecessary abstractions, and removed them.

There's plenty of prior art in the library for Base-based constructors accepting config properties that don't become attributes. I think we need to stop defaulting to everything being an attribute for no reason other than that it's the YUI way.

@rgrove

rgrove Feb 6, 2013

Contributor

An earlier version of Tree exposed attributes for rootNode and children that simply acted as getters for the properties for the sake of people who enjoy using abstractions for things that don't need abstractions.

But then I thought that maybe those people should learn not to use unnecessary abstractions, and removed them.

There's plenty of prior art in the library for Base-based constructors accepting config properties that don't become attributes. I think we need to stop defaulting to everything being an attribute for no reason other than that it's the YUI way.

This comment has been minimized.

@ericf

ericf Feb 6, 2013

Member

Those abstractions become useful and powerful when we look beyond today, and when thinking about this code living for years to come. Some of Tree's feature may benefit from being lazy and might want to leverage ES5/6 features in the future; to maintain compatibility with older environments these abstraction become highly valuable.

The general way I look at attribute vs. properties when developing Base-based classes is on a per-instance vs. class-hierarchy state/config. This combined with thinking towards encapsulating future language features behind the get/set() abstraction is what lead me to thinking that these should be attributes.

Sorry I wasn't more clear on my reasoning… leading you to think I was suggesting we add a layer of abstraction for the sake of it.

@ericf

ericf Feb 6, 2013

Member

Those abstractions become useful and powerful when we look beyond today, and when thinking about this code living for years to come. Some of Tree's feature may benefit from being lazy and might want to leverage ES5/6 features in the future; to maintain compatibility with older environments these abstraction become highly valuable.

The general way I look at attribute vs. properties when developing Base-based classes is on a per-instance vs. class-hierarchy state/config. This combined with thinking towards encapsulating future language features behind the get/set() abstraction is what lead me to thinking that these should be attributes.

Sorry I wasn't more clear on my reasoning… leading you to think I was suggesting we add a layer of abstraction for the sake of it.

This comment has been minimized.

@ericf

ericf Feb 14, 2013

Member

@rgrove any additional thoughts on properties vs. attributes for these two?

@ericf

ericf Feb 14, 2013

Member

@rgrove any additional thoughts on properties vs. attributes for these two?

This comment has been minimized.

@rgrove

rgrove Feb 14, 2013

Contributor

I'm not convinced these need to be attributes. It'd be slower, more verbose, and I don't see any benefit.

On Wednesday, February 13, 2013 at 10:47 PM, Eric Ferraiuolo wrote:

In src/tree/js/tree.js:

  • node class will be unique to this Tree instance and will not affect any > + other instances, nor will it modify the defined nodeClass itself. > + > + This provides a late-binding extension mechanism for nodes that doesn't > + require them to extend Y.Base, which would incur a significant performance > + hit. > + > + @Property {Array} nodeExtensions > + @default [] > + / > + nodeExtensions: [], > + > + / > + Root node of this Tree. > + > + @Property {Tree.Node} rootNode
    @rgrove (https://github.com/rgrove) any additional thoughts on properties vs. attributes for these two?


Reply to this email directly or view it on GitHub (https://github.com/yui/yui3/pull/429/files#r3009109).

@rgrove

rgrove Feb 14, 2013

Contributor

I'm not convinced these need to be attributes. It'd be slower, more verbose, and I don't see any benefit.

On Wednesday, February 13, 2013 at 10:47 PM, Eric Ferraiuolo wrote:

In src/tree/js/tree.js:

  • node class will be unique to this Tree instance and will not affect any > + other instances, nor will it modify the defined nodeClass itself. > + > + This provides a late-binding extension mechanism for nodes that doesn't > + require them to extend Y.Base, which would incur a significant performance > + hit. > + > + @Property {Array} nodeExtensions > + @default [] > + / > + nodeExtensions: [], > + > + / > + Root node of this Tree. > + > + @Property {Tree.Node} rootNode
    @rgrove (https://github.com/rgrove) any additional thoughts on properties vs. attributes for these two?


Reply to this email directly or view it on GitHub (https://github.com/yui/yui3/pull/429/files#r3009109).

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Feb 6, 2013

Member

@rgrove I think this component will be really useful, and it makes sense to me to have a YUI component which represents a standard data structure.

Beyond the comments I made inline, I have some other higher level comments:

Adds 9 New Modules

This component adds 9 new YUI modules, and I think it should be reduced down to 4. There is a pattern of Y.Tree extension modules which require Y.Tree.Node companion extension modules. These class extensions are usually small and I don't see why they shouldn't be defined together.

Here's how I would imagine organizing this component into modules:

  • tree: provides Tree and Tree.Node.
  • tree-labelable: provides Tree.Labelable and Tree.Node.Labelable.
  • tree-openable: provides Tree.Openable, Tree.Node.Openable (which has Plugin.Tree.Lazy's functionality.
  • tree-selectable: provides Tree.Selectable and Tree.Node.Selectable.

Adds Yet-Another-Inheritence-Mechanism

This component uses two inheritance mechanisms: the standard Y.Base features, and a new, sort-of-like Y.Base class extension mechanism. I understand that you don't want tree nodes to be Base-based objects, but I worry that having this new, but similar, class extension mechanism will cause confusion.

I would like use to see if we can avoid having to do that, and instead use Y.Base.build/mix() or extract the main (and common) parts of it such that it can be used with non-Base-based classes (simple constructors).

Member

ericf commented Feb 6, 2013

@rgrove I think this component will be really useful, and it makes sense to me to have a YUI component which represents a standard data structure.

Beyond the comments I made inline, I have some other higher level comments:

Adds 9 New Modules

This component adds 9 new YUI modules, and I think it should be reduced down to 4. There is a pattern of Y.Tree extension modules which require Y.Tree.Node companion extension modules. These class extensions are usually small and I don't see why they shouldn't be defined together.

Here's how I would imagine organizing this component into modules:

  • tree: provides Tree and Tree.Node.
  • tree-labelable: provides Tree.Labelable and Tree.Node.Labelable.
  • tree-openable: provides Tree.Openable, Tree.Node.Openable (which has Plugin.Tree.Lazy's functionality.
  • tree-selectable: provides Tree.Selectable and Tree.Node.Selectable.

Adds Yet-Another-Inheritence-Mechanism

This component uses two inheritance mechanisms: the standard Y.Base features, and a new, sort-of-like Y.Base class extension mechanism. I understand that you don't want tree nodes to be Base-based objects, but I worry that having this new, but similar, class extension mechanism will cause confusion.

I would like use to see if we can avoid having to do that, and instead use Y.Base.build/mix() or extract the main (and common) parts of it such that it can be used with non-Base-based classes (simple constructors).

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Feb 6, 2013

Contributor

This component adds 9 new YUI modules, and I think it should be reduced down to 4. There is a pattern of Y.Tree extension modules which require Y.Tree.Node companion extension modules. These class extensions are usually small and I don't see why they shouldn't be defined together.

Makes sense.

I would like use to see if we can avoid having to do that, and instead use Y.Base.build/mix() or extract the main (and common) parts of it such that it can be used with non-Base-based classes (simple constructors).

This is not currently feasible. If Base were performant enough for this use case, I'd have used it. If it were possible to use only part of it and get the performance I needed, I'd have used it. But I wasn't going to rearchitect Base just to meet the needs of Tree.Node.

The very simple class extension mechanism in Tree is essentially an extraction of the most fundamental concept of Base, but other than sharing a similar core concept, there's little relation between the two. This extension mechanism amounts to just a handful of code and allows Tree.Node to be extensible without becoming a heavyweight Base instance (and without waiting months or years for Base to be rearchitected).

If you're suggesting that we should extract this extension mechanism from Tree and make it generally available as a lightweight alternative to Base, I'd support that. But if you're suggesting that we add this use case to the long and ever-growing Base todo list, I'm not confident it will happen in a reasonable time frame, and I can't wait for it.

Contributor

rgrove commented Feb 6, 2013

This component adds 9 new YUI modules, and I think it should be reduced down to 4. There is a pattern of Y.Tree extension modules which require Y.Tree.Node companion extension modules. These class extensions are usually small and I don't see why they shouldn't be defined together.

Makes sense.

I would like use to see if we can avoid having to do that, and instead use Y.Base.build/mix() or extract the main (and common) parts of it such that it can be used with non-Base-based classes (simple constructors).

This is not currently feasible. If Base were performant enough for this use case, I'd have used it. If it were possible to use only part of it and get the performance I needed, I'd have used it. But I wasn't going to rearchitect Base just to meet the needs of Tree.Node.

The very simple class extension mechanism in Tree is essentially an extraction of the most fundamental concept of Base, but other than sharing a similar core concept, there's little relation between the two. This extension mechanism amounts to just a handful of code and allows Tree.Node to be extensible without becoming a heavyweight Base instance (and without waiting months or years for Base to be rearchitected).

If you're suggesting that we should extract this extension mechanism from Tree and make it generally available as a lightweight alternative to Base, I'd support that. But if you're suggesting that we add this use case to the long and ever-growing Base todo list, I'm not confident it will happen in a reasonable time frame, and I can't wait for it.

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Feb 6, 2013

Member

I understand that you don't want tree nodes to be Base-based objects, but I worry that having this new, but similar, class extension mechanism will cause confusion.

The very simple class extension mechanism in Tree is essentially an extraction of the most fundamental concept of Base, but other than sharing a similar core concept, there's little relation between the two. This extension mechanism amounts to just a handful of code and allows Tree.Node to be extensible without becoming a heavyweight Base instance (and without waiting months or years for Base to be rearchitected).

Yeah I get that (note the quote of myself above yours here), I wasn't suggesting that Tree.Node should extend Base.

If you're suggesting that we should extract this extension mechanism from Tree and make it generally available as a lightweight alternative to Base, I'd support that. But if you're suggesting that we add this use case to the long and ever-growing Base todo list, I'm not confident it will happen in a reasonable time frame, and I can't wait for it.

I'm suggesting the something closer to the former. With the idea that it would sit along side Y.extend(), and could be used under the hood to implement some parts of Y.Base.build/mix().

Member

ericf commented Feb 6, 2013

I understand that you don't want tree nodes to be Base-based objects, but I worry that having this new, but similar, class extension mechanism will cause confusion.

The very simple class extension mechanism in Tree is essentially an extraction of the most fundamental concept of Base, but other than sharing a similar core concept, there's little relation between the two. This extension mechanism amounts to just a handful of code and allows Tree.Node to be extensible without becoming a heavyweight Base instance (and without waiting months or years for Base to be rearchitected).

Yeah I get that (note the quote of myself above yours here), I wasn't suggesting that Tree.Node should extend Base.

If you're suggesting that we should extract this extension mechanism from Tree and make it generally available as a lightweight alternative to Base, I'd support that. But if you're suggesting that we add this use case to the long and ever-growing Base todo list, I'm not confident it will happen in a reasonable time frame, and I can't wait for it.

I'm suggesting the something closer to the former. With the idea that it would sit along side Y.extend(), and could be used under the hood to implement some parts of Y.Base.build/mix().

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Feb 6, 2013

Contributor

I'm suggesting the something closer to the former. With the idea that it would sit along side Y.extend(), and could be used under the hood to implement some parts of Y.Base.build/mix().

+1 to that, and I'd be willing to work on it. I don't want this to hold up Y.Tree though, since that's a whole new can of worms.

Contributor

rgrove commented Feb 6, 2013

I'm suggesting the something closer to the former. With the idea that it would sit along side Y.extend(), and could be used under the hood to implement some parts of Y.Base.build/mix().

+1 to that, and I'd be willing to work on it. I don't want this to hold up Y.Tree though, since that's a whole new can of worms.

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Feb 6, 2013

Member

I'm suggesting the something closer to the former. With the idea that it would sit along side Y.extend(), and could be used under the hood to implement some parts of Y.Base.build/mix().

+1 to that, and I'd be willing to work on it. I don't want this to hold up Y.Tree though, since that's a whole new can of worms.

Sounds good. Knowing this is a future thing we expect to do, we'll want to make sure the extension mechanism in Tree isn't too specific that it can be refactored to use the generic utility in the future.

Member

ericf commented Feb 6, 2013

I'm suggesting the something closer to the former. With the idea that it would sit along side Y.extend(), and could be used under the hood to implement some parts of Y.Base.build/mix().

+1 to that, and I'd be willing to work on it. I don't want this to hold up Y.Tree though, since that's a whole new can of worms.

Sounds good. Knowing this is a future thing we expect to do, we'll want to make sure the extension mechanism in Tree isn't too specific that it can be refactored to use the generic utility in the future.

@juandopazo

This comment has been minimized.

Show comment
Hide comment
@juandopazo

juandopazo Feb 14, 2013

Member

Any chance we can get API parity with WidgetParent/Child? It'd be nice if Tree had get('children'), item(0), etc and Tree.Node had next(), previous() and such.

Member

juandopazo commented Feb 14, 2013

Any chance we can get API parity with WidgetParent/Child? It'd be nice if Tree had get('children'), item(0), etc and Tree.Node had next(), previous() and such.

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Feb 14, 2013

Contributor

I don't see any reason for children to be an attribute.

next() and previous() would be useful. I'll add those.

item(0) is unnecessary because children is an array and can be accessed directly.

I'm leaving for a vacation today so I may be slow in responding to further comments, but keep 'em coming.

On Thursday, February 14, 2013 at 8:30 AM, Juan Ignacio Dopazo wrote:

Any chance we can get API parity with WidgetParent/Child? It'd be nice if Tree had get('children'), item(0), etc and Tree.Node had next(), previous() and such.


Reply to this email directly or view it on GitHub (#429 (comment)).

Contributor

rgrove commented Feb 14, 2013

I don't see any reason for children to be an attribute.

next() and previous() would be useful. I'll add those.

item(0) is unnecessary because children is an array and can be accessed directly.

I'm leaving for a vacation today so I may be slow in responding to further comments, but keep 'em coming.

On Thursday, February 14, 2013 at 8:30 AM, Juan Ignacio Dopazo wrote:

Any chance we can get API parity with WidgetParent/Child? It'd be nice if Tree had get('children'), item(0), etc and Tree.Node had next(), previous() and such.


Reply to this email directly or view it on GitHub (#429 (comment)).

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Feb 15, 2013

Contributor

@ericf I took a pass at implementing clone(), and I've decided not to jump down that hole right now. It'd require doing a deep clone on the data and state properties (or doing a JSON stringify/parse round-trip), which is a pain. I've saved my work in progress and can revisit it if there's a lot of interest in a clone() method, but for now I'm not sure it's worth the trouble.

Contributor

rgrove commented Feb 15, 2013

@ericf I took a pass at implementing clone(), and I've decided not to jump down that hole right now. It'd require doing a deep clone on the data and state properties (or doing a JSON stringify/parse round-trip), which is a pain. I've saved my work in progress and can revisit it if there's a lot of interest in a clone() method, but for now I'm not sure it's worth the trouble.

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Feb 15, 2013

Member

Sounds good

Member

ericf commented Feb 15, 2013

Sounds good

@ericf ericf merged commit 0387ccb into yui:dev-3.x Feb 15, 2013

1 check failed

default The Travis build failed
Details

@rgrove rgrove deleted the rgrove:tree branch Feb 15, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment