Skip to content
This repository has been archived by the owner on Feb 24, 2022. It is now read-only.

Add nodes to an existing instance #61

Merged
merged 4 commits into from
May 26, 2017
Merged

Add nodes to an existing instance #61

merged 4 commits into from
May 26, 2017

Conversation

Wikiki
Copy link
Contributor

@Wikiki Wikiki commented May 21, 2017

Add new nodes based on a selector to an existing instance.

Example:
$( '#div1' ).add( '#div2' )

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.8%) to 92.6% when pulling e071fad on Wikiki:feature/addNodes into e0991e2 on webpro:master.

@webpro
Copy link
Owner

webpro commented May 22, 2017

What functionality are you missing from .push() (array) and/or e.g. .append() (DOM)?

@Wikiki
Copy link
Contributor Author

Wikiki commented May 22, 2017

Hi,

I miss the chainability from push and using .append() does not works. If I log the DOMtastic instance after an .append() it's still empty.
And .push() function doesn't return a DOMtastic instance.

Example:

console.log( $().add('.sortable') );           // Display a list of all matching elements
console.log( $().push('.sortable') );         // Display 1
console.log( $().append('.sortable') );     // Display an empty array

regards,

@webpro
Copy link
Owner

webpro commented May 22, 2017

$(selector).concat(NodeList);
$('.first').concat($('.second'));
$().concat($('.sortable'));

@Wikiki
Copy link
Contributor Author

Wikiki commented May 22, 2017

Why not having a new .concat() method instead of a .add() one.

@webpro
Copy link
Owner

webpro commented May 22, 2017

Great idea. Could you please add specs and make sure the code style matches the rest of the project (npm run lint)?

@Wikiki
Copy link
Contributor Author

Wikiki commented May 22, 2017

Maybe you can help me, it's my first time with mocha.

When I run npm run test on my side it's working perfectly well but when it's run by Travis CI my test does not pass. I got the error TypeError: nodes.indexOf is not a function

@Wikiki
Copy link
Contributor Author

Wikiki commented May 22, 2017

The problem is only for npm run test:jquery
Any idea why indexOf doesn't work with jquery-compat test ?

export const concat = function( elements ) {
const nodes = this;
each( elements, element => {
if( nodes.indexOf( element ) === -1 ) {
Copy link
Owner

Choose a reason for hiding this comment

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

The jquery-compat mode has an index method (not indexOf), like jQuery itself; so you can't use this directly. Perhaps something like [].indexOf.call(this, element) is better here.

nodes.push( element );
}
} );
return $( nodes );
Copy link
Owner

Choose a reason for hiding this comment

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

Please return this; no need to create a new instance?

@Wikiki
Copy link
Contributor Author

Wikiki commented May 23, 2017

Thanks for your help.
I've made the corrections.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 94.243% when pulling 3bcea10 on Wikiki:feature/addNodes into e0991e2 on webpro:master.

@Wikiki
Copy link
Contributor Author

Wikiki commented May 23, 2017

Can I add the following code at the beginning of the method and still compliant with DOMtastic's philosophy:

if ( typeof elements === 'string' ) {
    elements = querySelector( elements );
  }

to be able to concat from a selector ?

@webpro webpro merged commit 3b17912 into webpro:master May 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants