Skip to content

Conversation

Saranya-Raaj
Copy link

Removed htmlbar void element test and covered it in curly components test.

Part of #13127

cc @chancancode

Copy link
Member

Choose a reason for hiding this comment

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

The original test was checking for childNodes of the components element, not for childViews.

I think this (and the one below) should be:

assert.deepEqual(fooBarInstance.element.childNodes.length, 0, 'no childNodes are added for `<input>`');

@rwjblue
Copy link
Member

rwjblue commented Mar 26, 2016

Small nit-pick, but this looks great, thank you!

@Saranya-Raaj Saranya-Raaj force-pushed the remove-void-component branch from 3645a4f to 4c6f156 Compare March 27, 2016 03:26
@Saranya-Raaj
Copy link
Author

@rwjblue Updated, Thanks

@Saranya-Raaj Saranya-Raaj force-pushed the remove-void-component branch from 4c6f156 to a751657 Compare March 27, 2016 04:51
@rwjblue
Copy link
Member

rwjblue commented Mar 27, 2016

👍 - LGTM

I'll leave to @chancancode to do final review + merge (I'm not super great at the I-N-U-R style yet 😸).

@homu
Copy link
Contributor

homu commented Mar 27, 2016

☔ The latest upstream changes (presumably #13190) made this pull request unmergeable. Please resolve the merge conflicts.

@Saranya-Raaj Saranya-Raaj force-pushed the remove-void-component branch from a751657 to c881eed Compare March 28, 2016 04:44
Copy link
Member

Choose a reason for hiding this comment

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

a small nit-pick, assert.equal is probably clearer here

Copy link
Author

Choose a reason for hiding this comment

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

👍 Done

Copy link
Member

Choose a reason for hiding this comment

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

When comparing against 0, it's always a good bet to use assert.strictEqual which uses === instead of == :)

@Saranya-Raaj Saranya-Raaj force-pushed the remove-void-component branch from c881eed to 88c6bf3 Compare March 28, 2016 16:42
Copy link
Member

Choose a reason for hiding this comment

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

Can ✂️ the {{yield}}

@chancancode
Copy link
Member

Looks good to me! Can be merged after resolving the minor comments above. Thank you @Matrixz!

@Saranya-Raaj Saranya-Raaj force-pushed the remove-void-component branch from 88c6bf3 to 03a96d4 Compare March 29, 2016 13:12
@chancancode chancancode merged commit 03a96d4 into emberjs:master Mar 31, 2016
chancancode added a commit that referenced this pull request Mar 31, 2016
[Glimmer2] Remove void element test
@chancancode
Copy link
Member

Thanks @Matrixz!

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.

5 participants