Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(modal): compile modal before inserting into DOM #5224

Closed
wants to merge 1 commit into from

Conversation

onumossn
Copy link
Contributor

I think this fixes #5183

@wesleycho
Copy link
Contributor

This looks like a good fix to me - thoughts @Foxandxss and/or @icfantv ?

@wesleycho wesleycho added this to the 1.0.4 milestone Jan 12, 2016
@Foxandxss
Copy link
Contributor

If it fixes it, I am all in, but we need to be sure (AKA plunker).

@icfantv
Copy link
Contributor

icfantv commented Jan 12, 2016

Looks like there's 13 broken tests in the build. Need to see if the tests are no longer valid or they need to be updated. @onumossn, if you would like this merged, your work is not yet complete.

@onumossn
Copy link
Contributor Author

The original commit, actually, only fixed issue 1 of the two listed on #5183, so I made additional updates to fix the 2nd issue.

@Foxandxss The cases can be tested from the demos, so is a plunker necessary?

@icfantv Sorry about that. I had used the wrong command to test initially. I updated the tests. Previously an extra $animate.flush was used because the $compile would use more $animates after the first $animate.flush in the open function, but since the $compile no longer waits for the first $animate, its no longer necessary.

@Foxandxss
Copy link
Contributor

A test to prove that your PR fixes the issue.

@icfantv
Copy link
Contributor

icfantv commented Jan 12, 2016

@onumossn, what @Foxandxss means is take the proper file from the dist folder and create a plunker using it. Easiest thing to do would be to use the modal plunker demo (from the docs page) remove the script include for UIBS, and add yours locally instead.

@Foxandxss, @wesleycho, IIRC we added those flush() calls when angular-animate changed between 1.4.4 and 1.4.5, right?

@wesleycho
Copy link
Contributor

The flush() has to do with ngAnimate.

What you could do is test for the newly created element's presence in the element being appended to (body by default) and then flush.

@onumossn
Copy link
Contributor Author

flush is being run in the open helper. However, flush needed to be run twice before because after the first flush the uib-modal-window directive gets compiled which adds more animation to the buffer which needs to be flushed again. In this version, the compile occurs before the first flush so all the animations are in the buffer when the first flush occurs in the open helper. I may not have been very clear in my last comment.

@onumossn
Copy link
Contributor Author

@@ -2,5 +2,5 @@
uib-modal-animation-class="fade"
modal-in-class="in"
ng-style="{'z-index': 1050 + index*10, display: 'block'}">
<div class="modal-dialog" ng-class="size ? 'modal-' + size : ''"><div class="modal-content" uib-modal-transclude></div></div>
<div class="modal-dialog {{size ? 'modal-' + size : ''}}"><div class="modal-content" uib-modal-transclude></div></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change made? This seems unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ngClass was going through the animation processes of all the before and after classes which was causing the size when picking small or large to be regular size when initially rendered and the jump to the correct size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, that's a negative of it being tied to $animate...but I guess it can't be avoided then.

@wesleycho
Copy link
Contributor

This needs a test still that the element is compiled before the $animate.enter completes.

@onumossn
Copy link
Contributor Author

Added tests


topModal = $uibModalStack.getTop();

expect(topModal.value.modalDomEl[0].querySelector('#test')).toBeNull();
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just do topModal.value.modalDomEl.find('#test') here I believe - we have jQuery in the testing situation.

@wesleycho
Copy link
Contributor

Once the query selectors are changed to just use jQuery, can you squash your commits into one?

This LGTM otherwise.

@onumossn
Copy link
Contributor Author

Update the tests to use jQuery. Sorry for the delay.

@wesleycho
Copy link
Contributor

LGTM - thanks for this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

display issues when opening modal
4 participants