Feature/dropdown add to body

[rysilva]

Fixes #1030

Look for dropdown-append-to-body attribute on the dropdown directive. If it's there, append the dropdown-menu to the body and show/hide/position it when the dropdown toggles.

[rysilva]

I'm attempting to remove the dropdown menu from the body when the main dropdown element fires the $destroy event. However, on the demo page the $destroy event never gets fired. I've been able to see the event get fired in my own application using this code, though.

In the tests, I do in fact see that the $destroy event gets fired if I call element.remove() and the code successfully calls dropdownMenu.remove(). However, in the test browser window the element never gets removed from the page. The remove call seems to do nothing.

Any ideas why?

[jessicawangdt]

+1!!!

[wesleycho]

This needs a test to fix the garbage collection.

[rysilva]

@wesleycho do you mean it needs a fix to test the garbage collection?

I had a test that looked like this:

it('removes the menu when the dropdown is removed', function() {
  element.remove();
  $rootScope.$digest();
  //Here, I am able to verify that line 80 (self.dropdownMenu.remove();) is in fact called but it has no effect in the test environment.
});

What I was referring to in my comment was that I was unable to verify the outcome of self.dropdownMenu.remove() because it remained in the DOM in the test runner's browser, even though I could verify that self.dropdownMenu was defined and referred to the correct element. I'll give it another shot, but I was hoping there might be some testing quirk that I didn't know about.

[wesleycho]

@rysilva if you're able to do so, I'm willing to review this again and merge when ready.

Perhaps element.remove() did not properly garbage collection the $scope and emit the $destroy event?

[jessicawangdt]

@rysilva, @wesleycho : looking forward to this fix, thanks to both of you.

[andyperlitch]

@rysilva and @wesleycho, I believe I understand what is happening: the element is not being cleaned up after each test. In other words, element.remove() needs to be called at the end of his test. This is what triggers the removal of the dropdown from the body. I would actually recommend calling element.remove() in an afterEach block at the top of this whole spec. A lot of tests are doing it in a one-off way anyway, and I see no reason to keep elements around after each test.

I have issued a pull request to @rysilva that not only fixes all this, but also resolves the conflicts with the current master preventing automatic merge. @wesleycho, assuming @rysilva merges my PR to his branch, please review again.

[rysilva]

Thanks, I'll take a look

[rysilva]

I'm not sure why adding an afterEach would solve the issue, because you're already calling element.remove() in the test anyway. So for that test the afterEach should have no effect, right?

I was going to run locally again but I'm currently getting an error running grunt:

Running "karma:continuous" (karma) task
[2015-04-01 08:07:20.330] [ERROR] config - Invalid config file!
 [ReferenceError: module is not defined]

[wesleycho]

You may need to run npm update.

Also the history needs to be cleaned up - the merge commit should not be there, and all of the commits should be squashed into one.

[rysilva]

Squashed all the commits.

[ghost]

Cannot wait for this feature to be merged! It fixes all my problems.

[wesleycho]

Merged - thanks for the good work @rysilva and @andyperlitch !

[andyperlitch]

all @rysilva... thanks @wesleycho!

[jessicawangdt]

@andyperlitch , @rysilva , @wesleycho : thanks a lot!!!!!!!!!!!!!!!!!!!!!