refactor(NgView): Make use of the ViewPort #1069
Conversation
| @@ -28,24 +28,27 @@ main() { | |||
|
|
|||
|
|
|||
| it('should switch template', async(() { | |||
| Element root = _.compile('<ng-view></ng-view>'); | |||
| Element root = _.compile('<div><ng-view></ng-view></div>'); | |||
There was a problem hiding this comment.
why do you wrap these in div?
There was a problem hiding this comment.
Because ngview need to have a parent for transclude to work as explained in the PR comment. Let me know of anything is unclear.
There was a problem hiding this comment.
actually let me try to put this in a clearer way:
Without the wrapping <div>, compile() would return the <ng-view> node.
It was perfectly fine before this PR because the nodes were added as children of <ng-view>.
Now that we use transclusion, the <ng-view> node is removed from the DOM and replaced by an anchor (<!--ANCHOR: ng-view-->). The view nodes are appended as siblings of this anchor the <ng-view> node is removed from the DOM at compile time and <ng-view>.text will always ever contain its content at compile time: "".
#Closes 704
|
I have updated the PR to use |
| var childScope = _scope; | ||
| _scope.rootScope.domWrite(() { | ||
| _viewPort.remove(view); | ||
| childScope.destroy(); |
There was a problem hiding this comment.
destroying scope in domWrite feels odd to me... but I can't say if that will cause issues... just pointing it out... no action required.
There was a problem hiding this comment.
destroying scope in domWrite is wrong. It should be destroyed immediately, and DOM operation delayed.
|
I don't know why it was reverted, don't think the bug was isolated... but this generally lgtm. |
| @@ -54,7 +54,7 @@ part of angular.routing; | |||
| @Decorator( | |||
| selector: 'ng-view', | |||
| module: NgView.module, | |||
| visibility: Directive.CHILDREN_VISIBILITY) | |||
| children: Directive.TRANSCLUDE_CHILDREN) | |||
There was a problem hiding this comment.
This change will not work. :-( The issue is that ng-view's are nestable and so they need ChildrenVisibility, otherwise nested routes do not work. The new DirectiveInjector makes it not possible to inject Transcluded controllers. We can change the behavior, but that is the current status. Can we talk about it.
I feel like ngView should be transcluding so that it can properly handle animation, and it is what we do in AngularJS. (https://github.com/angular/angular.js/blob/master/src/ngRoute/directive/ngView.js#L188)
There was a problem hiding this comment.
PING, can you address this change?
There was a problem hiding this comment.
I have this on my todo list, will look at it tomorrow
There was a problem hiding this comment.
The visibility has been removed because children visibility is the default value (Visibility.CHILDREN).
What kind of tests need to be added ? (We already have tests for nested ng-view.)
|
@vicb "tomorrow" was a long time ago! Status? |
|
needs work. delayed by higher priority tasks |
chirayuk
force-pushed
the
master
branch
2 times, most recently
from
8fd235c to
50e2645
Compare
|
@mhevery I want to tackle this long running PR. Could you please answer the question about the tests, I'll push an updated version after that (with the scope removal issue fixed) |
Closes 704
This was first introduced by #910 and later reverted in #991.
Some info about the change:
NgViewnow makes use of theViewPortinstead of appending / removing elements manually,<ng-view>is removed from the DOM in favor of an anchor (HTML comment),<ng-view>s are wrapped indivin the tests,_.rootScope.apply()is required to run the flush phase when the view are inserto to /removed from the DOM.@pavelgj @jbdeboer you have both commented the original PR, could you please review this one and let me know if you seen issues with it. Thanks.