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): node placed in incorrect order inside ngFor with ng-container #32324

Closed
wants to merge 1 commit into from

Conversation

@crisbeto
Copy link
Member

commented Aug 26, 2019

Fixes an issue where Ivy incorrectly inserts items in the beginning of an ngFor, if the ngFor is set on an ng-container. The issue comes from the fact that we choose the ng-container comment node as the anchor point before which to insert the content, however the node might be after any of the nodes inside the container. These changes switch to picking out the first node inside of the container instead.

@googlebot googlebot added the cla: yes label Aug 26, 2019
@ngbot ngbot bot modified the milestone: needsTriage Aug 26, 2019
@crisbeto crisbeto marked this pull request as ready for review Aug 26, 2019
@crisbeto crisbeto requested a review from angular/fw-core as a code owner Aug 26, 2019
@crisbeto crisbeto force-pushed the crisbeto:FW-1526/ivy-ng-for-order branch from 8bdac36 to 5025b2a Aug 27, 2019
@crisbeto crisbeto force-pushed the crisbeto:FW-1526/ivy-ng-for-order branch 2 times, most recently from 4cba14b to fef0e94 Sep 4, 2019
Fixes an issue where Ivy incorrectly inserts items in the beginning of an `ngFor`, if the `ngFor` is set on an `ng-container`. The issue comes from the fact that we choose the `ng-container` comment node as the anchor point before which to insert the content, however the node might be after any of the nodes inside the container. These changes switch to picking out the first node inside of the container instead.
@crisbeto crisbeto force-pushed the crisbeto:FW-1526/ivy-ng-for-order branch from fef0e94 to 1a759ec Sep 4, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Presubmits to get early results (to see how many targets we can resolve with this fix):

@kara
kara approved these changes Sep 5, 2019
Copy link
Contributor

left a comment

LGTM

@ngbot

This comment has been minimized.

Copy link

commented Sep 5, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: test_zonejs" is failing
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

FYI, I verified that the original issue is resolved. Blueprint and Global TAPs look good. Thank you.

@AndrewKushnir AndrewKushnir removed their assignment Sep 5, 2019
@matsko matsko closed this in da42a76 Sep 5, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…er (angular#32324)

Fixes an issue where Ivy incorrectly inserts items in the beginning of an `ngFor`, if the `ngFor` is set on an `ng-container`. The issue comes from the fact that we choose the `ng-container` comment node as the anchor point before which to insert the content, however the node might be after any of the nodes inside the container. These changes switch to picking out the first node inside of the container instead.

PR Close angular#32324
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…er (angular#32324)

Fixes an issue where Ivy incorrectly inserts items in the beginning of an `ngFor`, if the `ngFor` is set on an `ng-container`. The issue comes from the fact that we choose the `ng-container` comment node as the anchor point before which to insert the content, however the node might be after any of the nodes inside the container. These changes switch to picking out the first node inside of the container instead.

PR Close angular#32324
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 6, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.