Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

use document fragment for multiple top level elements #18

Merged
merged 1 commit into from

5 participants

@defunctzombie

When a string of multiple top level nodes is present, create a document
fragment to contain them.

<p>foo</p><p>bar</p> will result in a document fragment

@defunctzombie defunctzombie return document fragments for multiple top level nodes
When a string of multiple top level nodes is present, create a document
fragment to contain them.

<p>foo</p><p>bar</p> will result in a document fragment
e098182
@defunctzombie

Why not? this way you always get back one thing which you can add to the dom using appendChild. In the simple case where you only created one thing it will be exactly what you expect without some additional array stuff.

@jonathanong

you already always get back one thing that you expect: an array

it's also not consistent. i don't want to check what is returned each time to see whether it's a document fragment or not. easier just to loop through an array.

if you want to be consistent, always return elements in a document fragment

but I don't like this API because 1) it breaks backwards compatibility and 2) it's not what jQuery does, which a lot of us are used to (though this doesn't necessarily make it the best API)

@jonathanong

actually, an option would be nice.

domify(html) -> [] for backwards compatibilty
domify(html, true) -> documentFragment if you just want to append it.

but i don't think it should ever return just a single element.

@jb55

I pretty much always end up looping over the array to insert each element into a document fragment before I insert it into the DOM. When would you ever need an array? For an api that turns a string into dom elements a document fragment is the logical choice. The other way encourages poor use of DOM apis (multiple appendChild calls).

The documentFragment return option would be nice.

@jonathanong

I process all the elements in a loop before putting them in a document fragment then appending it.

I'm okay with returning document fragments, but this pr does it incorrectly. It should always return document fragments, not only if there are more than one elements.

@defunctzombie

Why not return single element in all cases? If we can return that element, great, if not, then return dom fragment?

@jonathanong

because i don't want to check the type of the result.

@jb55
@jkroso

+1 for returning a single element. I know my input string

@defunctzombie

You will know the type of the result. If you just did one element, it will be that. If you did multiple, it will be a fragment. You can get elements out of a fragment easily (no worse than some array) and for simple case of single element you will have that element.

I think you guys are focusing too much on some sort of "purity" versus the actual use cases.

@jonathanong

You will know the type of the result. If you just did one element, it will be that. If you did multiple, it will be a fragment.

You assume I know the input HTML string before passing it into domify. I don't. In my case, I render my HTML server-side and push it to the client.

@jkroso

sounds like we need two functions

@tj
Owner
tj commented

LGTM

@tj tj merged commit 1c18f76 into component:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 10, 2013
  1. @defunctzombie

    return document fragments for multiple top level nodes

    defunctzombie authored
    When a string of multiple top level nodes is present, create a document
    fragment to contain them.
    
    <p>foo</p><p>bar</p> will result in a document fragment
This page is out of date. Refresh to see the latest.
Showing with 27 additions and 36 deletions.
  1. +8 −16 index.js
  2. +19 −20 test/domify.js
View
24 index.js
@@ -45,7 +45,7 @@ function parse(html) {
if (tag == 'body') {
var el = document.createElement('html');
el.innerHTML = html;
- return [el.removeChild(el.lastChild)];
+ return el.removeChild(el.lastChild);
}
// wrap map
@@ -57,23 +57,15 @@ function parse(html) {
el.innerHTML = prefix + html + suffix;
while (depth--) el = el.lastChild;
- return orphan(el.children);
-}
-
-/**
- * Orphan `els` and return an array.
- *
- * @param {NodeList} els
- * @return {Array}
- * @api private
- */
-
-function orphan(els) {
- var ret = [];
+ var els = el.children;
+ if (els.length === 1) {
+ return el.removeChild(els[0]);
+ }
+ var fragment = document.createDocumentFragment();
while (els.length) {
- ret.push(els[0].parentNode.removeChild(els[0]));
+ fragment.appendChild(el.removeChild(els[0]));
}
- return ret;
+ return fragment;
}
View
39 test/domify.js
@@ -1,91 +1,90 @@
-var domify = require('domify');
+var assert = require('assert');
+var domify = require('../');
describe('domify(html)', function(){
it('should convert HTML to DOM elements', function(){
- var el = domify('<p>Hello</p>')[0];
+ var el = domify('<p>Hello</p>');
assert('P' == el.nodeName);
assert('Hello' == el.textContent);
var els = domify('<p>one</p><p>two</p><p>three</p>');
- assert('one' == els[0].textContent);
- assert('two' == els[1].textContent);
- assert('three' == els[2].textContent);
+ assert('onetwothree' == els.textContent);
})
it('should support body tags', function(){
- var el = domify('<body></body>')[0];
+ var el = domify('<body></body>');
assert('BODY' == el.nodeName);
})
it('should support body tags with classes', function(){
- var el = domify('<body class="page"></body>')[0];
+ var el = domify('<body class="page"></body>');
assert('BODY' == el.nodeName);
assert('page' == el.className);
})
it('should support legend tags', function(){
- var el = domify('<legend>Hello</legend>')[0];
+ var el = domify('<legend>Hello</legend>');
assert('LEGEND' == el.nodeName);
})
it('should support table tags', function(){
- var el = domify('<table></table>')[0];
+ var el = domify('<table></table>');
assert('TABLE' == el.nodeName);
})
it('should support thead tags', function(){
- var el = domify('<thead></thead>')[0];
+ var el = domify('<thead></thead>');
assert('THEAD' == el.nodeName);
})
it('should support tbody tags', function(){
- var el = domify('<tbody></tbody>')[0];
+ var el = domify('<tbody></tbody>');
assert('TBODY' == el.nodeName);
})
it('should support tfoot tags', function(){
- var el = domify('<tfoot></tfoot>')[0];
+ var el = domify('<tfoot></tfoot>');
assert('TFOOT' == el.nodeName);
})
it('should support caption tags', function(){
- var el = domify('<caption></caption>')[0];
+ var el = domify('<caption></caption>');
assert('CAPTION' == el.nodeName);
})
it('should support col tags', function(){
- var el = domify('<col></col>')[0];
+ var el = domify('<col></col>');
assert('COL' == el.nodeName);
})
it('should support td tags', function(){
- var el = domify('<td></td>')[0];
+ var el = domify('<td></td>');
assert('TD' == el.nodeName);
})
it('should support th tags', function(){
- var el = domify('<th></th>')[0];
+ var el = domify('<th></th>');
assert('TH' == el.nodeName);
})
it('should support tr tags', function(){
- var el = domify('<tr></tr>')[0];
+ var el = domify('<tr></tr>');
assert('TR' == el.nodeName);
})
it('should support option tags', function(){
- var el = domify('<option></option>')[0];
+ var el = domify('<option></option>');
assert('OPTION' == el.nodeName);
})
it('should support optgroup tags', function(){
- var el = domify('<optgroup></optgroup>')[0];
+ var el = domify('<optgroup></optgroup>');
assert('OPTGROUP' == el.nodeName);
})
it('should not set parentElement', function() {
- var el = domify('<p>Hello</p>')[0];
+ var el = domify('<p>Hello</p>');
assert(!el.parentElement);
assert(!el.parentNode);
})
Something went wrong with that request. Please try again.