Opened 13 years ago
Closed 7 years ago
#9406 closed enhancement (patchwelcome)
Modify NodeList.adopt() to be more in line with the rest of NodeList methods.
Reported by: | Eugene Lazutkin | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | low | Milestone: | 1.13 |
Component: | Core | Version: | 1.3.0 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description (last modified by )
In #9362 jburke proposes to change NodeList.adopt()
like that:
adopt: function(/*String||Array||DomNode*/ queryOrListOrNode, /*String?*/ position){ return this.addContent(d.query(queryOrListOrNode), position); }
It introduces two changes:
- This way it returns the main
NodeList
as other methods. - It copies the
NodeList
argument, instead of moving it.
Attachments (1)
Change History (8)
comment:1 Changed 13 years ago by
Description: | modified (diff) |
---|
comment:2 follow-ups: 3 4 Changed 13 years ago by
Description: | modified (diff) |
---|---|
Type: | defect → enhancement |
Changed 13 years ago by
Attachment: | place.html added |
---|
comment:3 Changed 13 years ago by
Replying to elazutkin:
Obviously I meant the aliasing metaphorically as "the same effect".
comment:4 Changed 13 years ago by
Replying to elazutkin:
This change effectively aliases
adopt
toaddContent
:dojo.NodeList.adopt = dojo.NodeList.addContent;While it is laudable, because it reduces the codebase, I am not so sure about James' point number 2: copying DOM nodes instead of moving them. Without this functionality there is no need for
adopt()
.
Well there are some slight differences, addContent cannot take a string that represents a DOM query, because of the possible conflict with the desire to insert a text node. I think you know that, based on your last comment as meaning metaphorically aliasing, but just making it explicit.
As to the copy of nodes -- it would only clone nodes if the current NodeList? had more than one node, otherwise it just does a move. So I think for most cases where adopt would be used now, there would be no functional difference -- if the node list has only one node in it, the adoption moves the new nodes under the only node in the NodeList?.
I believe NodeList? operations are group operations, so adding/moving content should work if you have multiple nodes in the current NodeList?. If just one node in the current node list, the new nodes should just be moved/inserted. However, if more than one node in the current node list, the first node gets the original new nodes, the other nodes get clones of the new nodes.
For what it is worth, I believe this is also necessary for jquery compatibility, and the original reason I used that approach for the new NodeList? mixins I added. I also think it makes sense given that the modify operations for a NodeList? should operate in some way on all the nodes.
You can see this approach used for methods that use NodeList?._place. Methods that use NodeList?._place moves/uses the new nodes directly if just one element in the current NodeList?, but specify a cloneNode for nodes in the current NodeList? at index greater than 0.
Which reminds me of another deficiency in
place()
: it behaves strangely with differentposition
arguments. Intuitively I expect that all elements will be inserted as one fragment in a position of my choosing. In practice:
I believe the private dojo._place does the right thing with the nodes. I want to switch NodeList?.place over to use NodeList?._place internally to fix the problem you mention and so we get the proper "move for first node, cloneNode for rest" behavior described above. So it might look like this:
place: function(/*String||Node||NodeList*/ queryOrNode, /*String*/ position){ var content = typeof queryOrNode == "string" || queryOrNode.nodeType ? dojo.query(queryOrNode) : queryOrNode; for(var i = 0, node; node = this[i]; i++){ this._place(content, node, position, i > 0); } return this; }, adopt: function(/*String||Array||DomNode*/ queryOrListOrNode, /*String?*/ position){ d.query(queryOrNode).place(this, position); return this; // NodeList },
Right now I favor those two implementations of place and adopt. What do you think?
comment:5 Changed 13 years ago by
Owner: | changed from anonymous to Eugene Lazutkin |
---|---|
Status: | new → assigned |
comment:6 Changed 10 years ago by
Priority: | high → low |
---|
comment:7 Changed 7 years ago by
Milestone: | future → 1.12 |
---|---|
Resolution: | → patchwelcome |
Status: | assigned → closed |
Given that no one has shown interest in creating a patch in the past 5+ years, I'm closing this as patchwelcome.
This change effectively aliases
adopt
toaddContent
:While it is laudable, because it reduces the codebase, I am not so sure about James' point number 2: copying DOM nodes instead of moving them. Without this functionality there is no need for
adopt()
. We can preserve it by moving the functionality in another direction:NodeList.place()
:Which reminds me of another deficiency in
place()
: it behaves strangely with differentposition
arguments. Intuitively I expect that all elements will be inserted as one fragment in a position of my choosing. In practice:I cannot believe that this is the intentional behavior. The test file is included.