Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

Update Node.js #265

Merged
merged 1 commit into from
Oct 9, 2013
Merged

Update Node.js #265

merged 1 commit into from
Oct 9, 2013

Conversation

labriola
Copy link
Contributor

@labriola labriola commented Oct 8, 2013

Inverted hasChildNodes() logic to ensure that the existance of a firstChild means there are childNodes

@arv
Copy link
Contributor

arv commented Oct 8, 2013

Thanks,

2 things.

  1. Did you sign the CLA? https://github.com/Polymer/polymer/blob/master/CONTRIBUTING.md
  2. Do you mind adding a test to make sure I don't break this again?

@labriola
Copy link
Contributor Author

labriola commented Oct 8, 2013

CLA is signed and filed.

I will add the test next and get this submitted too.

@labriola
Copy link
Contributor Author

labriola commented Oct 8, 2013

Test added to this branch. Want me to open a new pull request or sufficient?

var div = document.createElement('div');

//No children yet
assert( !div.hasChildNodes(), "should be false with no children" );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be a bit nit picky here. We follow http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml

  • No space after (
  • No space before )
  • Use single quotes

Copy link
Contributor

Choose a reason for hiding this comment

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

Use assert.isTrue instead so that this fails for thruthy values that are not boolean

@labriola
Copy link
Contributor Author

labriola commented Oct 9, 2013

No problem at all. I will fix these and resubmit.

If there is anything else just let me know as I intend on submitting a bunch of small patches over the next few weeks so any time we spend now will likely reduce time later.

@labriola
Copy link
Contributor Author

labriola commented Oct 9, 2013

Okay, updates. I ended up using assert.isTrue() and assert.isFalse(). Wasn't sure from your comment if that is acceptable or if you only wanted me to you isTrue(). Let me know if you want this revised and I will take a read through your code guidelines. Thanks.

test('hasChildNodes without a shadow root', function() {
var div = document.createElement('div');

// No children yet
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for these comments. Comments should explain why and not what.

@arv
Copy link
Contributor

arv commented Oct 9, 2013

LGTM

Can you squash these into a single commit?

(there might be one more churn tomorrow, I have to look into our policy related to the AUTHORS file)

@labriola
Copy link
Contributor Author

labriola commented Oct 9, 2013

Sure, just let me know tomorrow after you check on the final details and I will wrap it up into a single commit and get it through.

…tChild means there are childNodes. Added test to verify hasChildNodes() change with formatting per guidelines.
@labriola
Copy link
Contributor Author

labriola commented Oct 9, 2013

Now a single commit

arv added a commit that referenced this pull request Oct 9, 2013
@arv arv merged commit 5bb221b into googlearchive:master Oct 9, 2013
@arv
Copy link
Contributor

arv commented Oct 9, 2013

Thanks a lot

@labriola labriola deleted the patch-1 branch October 9, 2013 15:47
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.

2 participants