-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): properly get root nodes from embedded views with <ng-content> #36051
fix(core): properly get root nodes from embedded views with <ng-content> #36051
Conversation
c07a62f
to
3fc24ff
Compare
bb22920
to
5aac6fc
Compare
5aac6fc
to
27f9534
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @pkozlowski-opensource! This is awesome. Looking forward to its merge.
c296f72
to
b9d5a68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just comment nits
const projectionSlots = componentHost.projection; | ||
const slotIdx = tNode.projection as number; | ||
|
||
if (projectionSlots !== null && projectionSlots.length > slotIdx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining this condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the code - turns out that most of checks were not necessary (added an assert). This code is gone so things should be easier to reason about.
if (Array.isArray(nodesInSlot)) { | ||
result.push(...nodesInSlot); | ||
} else { | ||
const parentView = getLViewParent(componentView); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments what is going on here would be helpfull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the code and added an assert that should serve as documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with nits
1a420b0
to
444ec96
Compare
This commit fixes 2 separate issues related to root nodes retrieval from embedded views with `<ng-content>`: 1) we did not account for the case where there were no projectable nodes for a given `<ng-content>`; 2) we did not account for the case where projectable nodes for a given `<ng-content>` were represented as an array of native nodes (happens in the case of dynamically created components with projectable nodes); Fixes angular#35967
444ec96
to
85080f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
merge-assistance: can't remove the pending review |
…nt> (#36051) This commit fixes 2 separate issues related to root nodes retrieval from embedded views with `<ng-content>`: 1) we did not account for the case where there were no projectable nodes for a given `<ng-content>`; 2) we did not account for the case where projectable nodes for a given `<ng-content>` were represented as an array of native nodes (happens in the case of dynamically created components with projectable nodes); Fixes #35967 PR Close #36051
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…nt> (angular#36051) This commit fixes 2 separate issues related to root nodes retrieval from embedded views with `<ng-content>`: 1) we did not account for the case where there were no projectable nodes for a given `<ng-content>`; 2) we did not account for the case where projectable nodes for a given `<ng-content>` were represented as an array of native nodes (happens in the case of dynamically created components with projectable nodes); Fixes angular#35967 PR Close angular#36051
This commit fixes 2 separate issues related to root nodes retrieval from
embedded views with
<ng-content>
:we did not account for the case where there were no projectable nodes
for a given
<ng-content>
;we did not account for the case where projectable nodes for a given
<ng-content>
were represented as an array of native nodes (happens inthe case of dynamically created components with projectable nodes);
Fixes #35967