Skip to content
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(ivy): properly insert views into ViewContainerRef injected by querying <ng-container> #33816

Closed

Conversation

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Nov 14, 2019

When asking for a ViewContainerRef on we do reuse comment
node as a LContainer's anchor. Before this fix the act of re-using a 's
comment node would result in this comment node being re-appended to the DOM in the wrong
place. With the fix in this PR we make sure that re-using 's comment node
doesn't result in unwanted DOM manipulation (ng-container's comment node is already part
of the DOM and doesn't have to be re-created / re-appended).

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 14, 2019

@pkozlowski-opensource thanks for the fix, the changes LGTM and I've started g3 presubmits:

I think it'd be great if @mhevery can have a look as well (so I left the "review" label).

Thank you.

} else {
appendChild(commentNode, hostTNode, hostView);
// A container can be created on the root (topmost / bootstrapped) component and in this case
// we can't use LTree to insert container's marker node (both parent of a comment node and the

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 14, 2019

Member

LTree is not a type we have in system, neither is LNode What did you have in mind here. RNode???

I am a bit confused, so maybe an example would make this more clear?

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 14, 2019

Author Member

Oh, this sounds like an early, pre-existing comment, I should have checked it when moving the code... Thnx, will clean-up!

appendChild(commentNode, hostTNode, hostView);
// A container can be created on the root (topmost / bootstrapped) component and in this case
// we can't use LTree to insert container's marker node (both parent of a comment node and the
// commend node itself is located outside of elements hold by LTree). In this specific case we

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 14, 2019

Member

same here.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 15, 2019

New set of g3 presubmits:

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 15, 2019

FYI, VE and Ivy g3 presubmits look good 👍

…rying <ng-container>

When asking for a ViewContainerRef on <ng-container> we do reuse <ng-container> comment
node as a LContainer's anachor. Before this fix the act of re-using a <ng-container>'s
comment node would result in this comment node being re-appended to the DOM in the wrong
place. With the fix in this PR we make sure that re-using <ng-container>'s comment node
doesn't result in unwanted DOM manipulation (ng-gontainer's comment node is already part
of the DOM and doesn't have to be re-created / re-appended).
@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:gh_33679 branch from e897b53 to 43cf0eb Nov 15, 2019
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member Author

pkozlowski-opensource commented Nov 18, 2019

merge-assistance due to " Status "google3" is pending"

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 18, 2019

@pkozlowski-opensource I've updated the status of g3. I did ran presubmits a few days ago and the result looks good. Thank you.

alxhub added a commit that referenced this pull request Nov 19, 2019
…rying <ng-container> (#33816)

When asking for a ViewContainerRef on <ng-container> we do reuse <ng-container> comment
node as a LContainer's anachor. Before this fix the act of re-using a <ng-container>'s
comment node would result in this comment node being re-appended to the DOM in the wrong
place. With the fix in this PR we make sure that re-using <ng-container>'s comment node
doesn't result in unwanted DOM manipulation (ng-gontainer's comment node is already part
of the DOM and doesn't have to be re-created / re-appended).

PR Close #33816
@alxhub alxhub closed this in 49c9f78 Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.