Skip to content
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

Adding prepend function on metal-dom #306

Closed
wants to merge 11 commits into from
Closed

Conversation

diegonvs
Copy link
Contributor

@diegonvs diegonvs commented Nov 21, 2017

Correcting #304 to solve issue #296

@diegonvs diegonvs changed the base branch from master to develop November 21, 2017 17:20
@diegonvs diegonvs changed the title Adding test cases for prepend Adding prepend function on metal-dom Nov 21, 2017
@diegonvs diegonvs mentioned this pull request Nov 21, 2017
Copy link

@robframpton robframpton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @diegonvs

Can you add a test for passing a node list? Similar to what we do for the append method https://github.com/metal/metal.js/blob/master/packages/metal-dom/test/dom.js#L201-L209.

Another thought I had is how we loop over items in this function. Let's say we have a NodeList like <div>el1</div>, <div>el2</div>, <div>el3</div> and we pass it to this method, the NodeList will essentially get reversed because it adds each element as the first child. Then you would get something like...

<div id="parent">
    <div>el3</div>
    <div>el2</div>
    <div>el1</div>
</div>

I think this behavior would be confusing, so let's loop through the NodeList backwards so that we preserve order.

@diegonvs
Copy link
Contributor Author

Thanks for review @Robert-Frampton, i'll preserve order on that tests

parent.appendChild(childArr[i]);
}
} else {
parent.insertBefore(child, parent.firstChild);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When calling this on a parent that has no children, this block will run even though line 537 has already added the child.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Robert-Frampton , what do you think that function return parent? User probably can get info about childNodes easily, instead actually returning child. I've sent a commit that fixes the problem on running though and returns parent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @diegonvs, we should probably keep it the same as the dom.append method which returns the child.

if (isNodeListLike(child)) {
const childArr = Array.prototype.slice.call(child);
for (let i = 0; i < childArr.length; i++) {
parent.appendChild(childArr[i]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add a node list to a parent that already has a child this won't work, this test will currently fail as an example.

        it('should prepend a node list to parent element', function() {
            let parent = document.createElement('div');
            let child = document.createElement('div');

            dom.addClasses(child, 'child');
            dom.append(parent, child);

            let childFrag = dom.buildFragment(
                '<div class="myChild">el1</div><div class="myChild2">el2</div><div class="myChild3">el3</div>'
            );

            dom.prepend(parent, childFrag.childNodes);

            assert.strictEqual(4, parent.childNodes.length);
            assert.strictEqual('myChild', parent.childNodes[0].className);
            assert.strictEqual('myChild2', parent.childNodes[1].className);
            assert.strictEqual('myChild3', parent.childNodes[2].className);
            assert.strictEqual('child', parent.childNodes[3].className);
        });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've corrected this using insertBefore instead appendChild

@robframpton
Copy link

Thanks @diegonvs! I've sent a follow up PR with some small formatting changes at #312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants