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.

Sign up for GitHub

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

Jump to bottom

Ng container shallow query #35384

Closed
wants to merge 1 commit into from
Closed

Ng container shallow query #35384

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Feb 12, 2020 •
edited

Fixes #34768

@pullapprove pullapprove bot requested a review from alxhub February 12, 2020 15:28
@kara kara added the area: core Issues related to the framework runtime label Feb 13, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 13, 2020
@pkozlowski-opensource pkozlowski-opensource force-pushed the ng_container_shallow_query branch 4 times, most recently from c4a6655 to 1f81c53 Compare February 13, 2020 14:06
@pkozlowski-opensource pkozlowski-opensource added target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy and removed target: patch This PR is targeted for the next patch release state: WIP labels Feb 13, 2020
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review February 13, 2020 14:07
@pullapprove pullapprove bot requested a review from IgorMinar February 13, 2020 14:08
@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 13, 2020
@mhevery
Copy link
Contributor

mhevery commented Feb 13, 2020

Adding cleanup to consider adding additional test, LGTM otherwise.

@kara kara requested review from kara and removed request for alxhub February 14, 2020 00:53
kara
kara requested changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Mostly ready to go, but can we also add more context to the commit message about what this change is and why?

aio/scripts/_payload-limits.json Outdated Show resolved Hide resolved
packages/core/test/acceptance/query_spec.ts Show resolved Hide resolved
packages/core/test/acceptance/query_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/query_spec.ts Show resolved Hide resolved
packages/core/src/render3/query.ts Show resolved Hide resolved
@pkozlowski-opensource
fix(ivy): queries should match elements inside ng-container with the …
8b04f08 
…descendants: false option

Before this change content queries with the `descendants: false` option, as implemented in ivy,
would not descendinto `<ng-container>` elements. This behaviour was different from the way the
View Engine worked. This change alligns ngIvy and VE behaviours when it comes to queries and the
`<ng-container>` elements and fixes a common bugs where a query target was placed inside the
`<ng-container>` element with a * directive on it.

Before:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node would NOT match -->
  </ng-container>
</needs-target>
```

After:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node WILL match -->
  </ng-container>
</needs-target>
```

Fixes angular#34768
@pkozlowski-opensource pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 18, 2020
kara
kara approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@kara kara removed the request for review from IgorMinar February 18, 2020 18:42
@kara kara added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 18, 2020
@kara
Copy link
Contributor

kara commented Feb 18, 2020

presubmit

@kara kara added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Feb 18, 2020
alxhub pushed a commit that referenced this pull request Feb 19, 2020
@pkozlowski-opensource @alxhub
fix(ivy): queries should match elements inside ng-container with the …
fd4ce84 
…descendants: false option (#35384)

Before this change content queries with the `descendants: false` option, as implemented in ivy,
would not descendinto `<ng-container>` elements. This behaviour was different from the way the
View Engine worked. This change alligns ngIvy and VE behaviours when it comes to queries and the
`<ng-container>` elements and fixes a common bugs where a query target was placed inside the
`<ng-container>` element with a * directive on it.

Before:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node would NOT match -->
  </ng-container>
</needs-target>
```

After:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node WILL match -->
  </ng-container>
</needs-target>
```

Fixes #34768

PR Close #35384
@alxhub alxhub closed this in 3f4e02b Feb 19, 2020
@angular-automatic-lock-bot
Copy link

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 Mar 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@kara kara kara approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

[ivy] ContentChildren doesn't pick up elements inside ng-container
4 participants
@pkozlowski-opensource @mhevery @kara @googlebot