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
Reuse dom.childElements and dom.childNodes #3145
Reuse dom.childElements and dom.childNodes #3145
Conversation
@cramforce Could you also have a look? |
Anyone know why this is failing? |
* These nodes can include Text, Comment and other child nodes. | ||
* @param {!Node} parent | ||
* @param {function(!Node):boolean} callback | ||
* @return {!Array.<!Node>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This looks good thanks! Few comments. |
a4f991c
to
f384d93
Compare
* @return {!Array<!Node>} | ||
*/ | ||
export function childNodes(parent, callback) { | ||
const childnodes = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: call this children
or nodes
. childnodes
doesn't follow the camel-case style and I understand why you didn't use the camel-case here - function name similarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
One more nit! otherwise LGTM |
@@ -143,6 +143,20 @@ describe('DOM', () => { | |||
.to.be.equal(0); | |||
}); | |||
|
|||
it('childNodes should find all matches', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a not-all-positive test as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
LGTM from my side. |
👍 LGTM, please wait for the tests and then merge! |
Fixes #3115