Nodes per element #51
Conversation
* Changed how `<tr>` elements are read from a named template (original code did not work in IE<10). * Do not use cloneNode or delete script tag based templates. This breaks the templates in IE<9. * Used `ko.utils.arrayMap` instead of `.map` since the latter is unsupported in IE<9. * Removed hanging commas that cause syntax errors in IE<8. These changes allow the ko-pre-rendered bindings to support the same browser versions as knockout itself.
…es should be associated with a single element in the array. * If unspecified, the default nodesPerElement is 1 * Modified "makeTemplateNode" to support multiple nodes in the template. * Modified the "existing" method so it correctly applies bindings when nodesPerElement > 1. * Modified the "createElements" method so it creates the correct number of array elements for the number of existing HTML nodes.
Tested: * Initialization of the observable array based on existing markup. * Proper rendering of multi-node templates when adding new items to the observable array. * Proper removal of markup when removing items from the observable array.
knockout-pre-rendered.js
Outdated
| @@ -78,20 +78,34 @@ | |||
| // For e.g. <template> tags | |||
| parentNode = sourceNode.content; | |||
| } else if (sourceNode.tagName === 'SCRIPT') { | |||
| parentNode = sourceNode.innerHTML.match(/<tr[\s\S]*?<\/tr>/g) ? document.createElement('tbody') : document.createElement('div'); | |||
| parentNode.innerHTML = sourceNode.innerHTML; | |||
| if(sourceNode.innerHTML.match(/<tr[\s\S]*?<\/tr>/g)) { | |||
There was a problem hiding this comment.
Very small nit: there should be a space after the if.
knockout-pre-rendered.js
Outdated
|
|
||
| for(var i = 0, node = template; i < nodesPerElement; i++) { |
There was a problem hiding this comment.
There should be a space after the for.
knockout-pre-rendered.js
Outdated
| var currentNode = node; | ||
| container.insertBefore(node.cloneNode(true), null); | ||
| // Find next element sibling. (Some older browsers don't support nextElementSibling). | ||
| do { node = node.nextElementSibling || node.nextSibling; } while(node && node.nodeType !== 1); |
There was a problem hiding this comment.
I would prefer to put this on multiple lines:
do {
node = node.nextElementSibling || node.nextSibling;
} while (node && node.nodeType !== 1);
knockout-pre-rendered.js
Outdated
| // Remove current template node | ||
| if(deleteTemplateNodes) { | ||
| ko.removeNode(currentNode); | ||
| } |
There was a problem hiding this comment.
There appears to be some indentation problem here
|
Should be all set. |
|
Merged! Thanks a lot @revengineering. I'll put out a new release tonight. |
|
My pleasure. Thanks for making this library! It has completely changed my perspective on knockout, and put it back in the running for my current and future projects. |
|
@revengineering That's great to hear! I've just published version 0.9.2 to NPM and GitHub, which includes your changes. Again, thanks a lot! |
|
This update seems to break foreachInit if your looped elements (if that's the right term) are tables, so something like: nets you the error I wrote a test for this, and I fixed it by changing line 81:
to
but I'm not sure if this is the best way to fix this? I can make a new PR with the test/fix if you'd like, though. |
That would be great! |
|
The fix you proposed would work except if the tr elements contain any nested tables I suggest an alternate regex of:
This way, it would only concern itself with templates where the first child is a tr element, regardless of whatever else might be inside them. (Also ignoring case, because browsers still understand uppercase tag names.) |
|
@penguinstampede The suggestion by @revengineering is probably good to follow, as the internals are better understood by @revengineering than by me currently :D |
Fixes Issue #50, allowing the body of the foreachInit to have multiple HTML nodes per array element.
Also fixes a couple of minor browser compatibility issues with older versions of IE.