Skip to content

fix(table): not picking up indirect descendant defs #17340

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
Merged
merged 1 commit into from
Oct 25, 2019
Merged

fix(table): not picking up indirect descendant defs #17340

merged 1 commit into from
Oct 25, 2019

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Oct 9, 2019

Fixes the table not picking descendants that aren't direct descendants. Currently this isn't a huge problem, however with Ivy it'll become an issue that'll prevent consumers from setting an ngIf around the defs.

Fixes #17339.

@crisbeto
fix(table): not picking up indirect descendant defs
e1788e5 
Fixes the table not picking descendants that aren't direct descendants. Currently this isn't a huge problem, however with Ivy it'll become an issue that'll prevent consumers from setting an `ngIf` around the defs.

Fixes angular#17339.
@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Oct 9, 2019
@crisbeto crisbeto requested a review from andrewseguin as a code owner October 9, 2019 13:57
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 9, 2019
crisbeto
crisbeto commented Oct 9, 2019
View reviewed changes
src/cdk/table/table.ts
@@ -375,16 +375,20 @@ export class CdkTable<T> implements AfterContentChecked, CollectionViewer, OnDes
* The column definitions provided by the user that contain what the header, data, and footer
* cells should render for each column.
*/
@ContentChildren(CdkColumnDef) _contentColumnDefs: QueryList<CdkColumnDef>;
@ContentChildren(CdkColumnDef, {descendants: true}) _contentColumnDefs: QueryList<CdkColumnDef>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this makes assumptions that we can't nest tables. AFAIK it's not something we support anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Right, based on the trick we use the get cell outlets after stamping out row views, we absolutely, 100% do not support nested tables.

jelbourn
jelbourn reviewed Oct 9, 2019
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Why would someone put an ngIf around a def? My immediate reaction would be that we would want to discourage that

src/cdk/table/table.ts
@@ -375,16 +375,20 @@ export class CdkTable<T> implements AfterContentChecked, CollectionViewer, OnDes
* The column definitions provided by the user that contain what the header, data, and footer
* cells should render for each column.
*/
@ContentChildren(CdkColumnDef) _contentColumnDefs: QueryList<CdkColumnDef>;
@ContentChildren(CdkColumnDef, {descendants: true}) _contentColumnDefs: QueryList<CdkColumnDef>;
Copy link
Member

Choose a reason for hiding this comment

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

Right, based on the trick we use the get cell outlets after stamping out row views, we absolutely, 100% do not support nested tables.

@crisbeto
Copy link
Member Author

crisbeto commented Oct 9, 2019

You can see an example in initial report: #17339.

@jelbourn
Copy link
Member

jelbourn commented Oct 9, 2019

It seems strange to me, since I I expect people to accomplish things like this either by swapping columns or by putting an ngIf inside the cell. Is there some advantage to this approach that I'm not thinking of?

@crisbeto
Copy link
Member Author

crisbeto commented Oct 9, 2019

I'm not sure really. I suppose it's more declarative? It's still something that works with ViewEngine but will break in Ivy.

jelbourn
jelbourn approved these changes Oct 9, 2019
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

I chatted w/ @andrewseguin and we agreed this is for the best

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P2 The issue is important to a large percentage of users, with a workaround labels Oct 9, 2019
@jelbourn
Copy link
Member

Three failing targets in Google, I will investigate today

@mmalerba mmalerba added this to the 9.0.0 milestone Oct 15, 2019
@jelbourn
Copy link
Member

Two of the targets were projects that had a nested mat-table. Reached out to the teams and they'll hopefully fix this week. Looking into the third failure now.

@jelbourn
Copy link
Member

The third failure was an instance of the table literally defining each matColumnDef twice; I sent a change to remove the extra one.

@surajtambe
Copy link

Solution for #17339 is to take checkbox outside of *ngfor , it will work as expected. I have implemented in similar way for adding checkbox for MatTable with Ivy Enabled. if checkbox column present in columns Aray it will render.

For above approch i have small questions,

  1. Does above approch adds performance hit ?
  2. Does this close doors for nested tables ?
  3. Solution i have added is desired functionality or workaround ?

I am new to open source contribution, not sure whether to comment on PR or else please Ignore.

@surajtambe
Copy link

Solution for #17339 is to take checkbox outside of *ngfor , it will work as expected. I have implemented in similar way for adding checkbox for MatTable with Ivy Enabled. if checkbox column present in columns Aray it will render.

For above approch i have small questions,

  1. Does above approch adds performance hit ?
  2. Does this close doors for nested tables ?
  3. Solution i have added is desired functionality or workaround ?

I am new to open source contribution, not sure whether to comment on PR or else please Ignore.

As my solution not working for #17339 , we should merge this PR to solve #17339

@mmalerba mmalerba merged commit 6982054 into angular:master Oct 25, 2019
@GlauberF
Copy link

GlauberF commented Oct 28, 2019 •
edited
Loading

any predictions to release this fix #17339 ?

@jelbourn
Copy link
Member

Should be in the first 9.0.0 RC

@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 Nov 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees

@jelbourn jelbourn

@andrewseguin andrewseguin

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

Ivy true, error matRowDef/matColumnDef
7 participants
@crisbeto @jelbourn @surajtambe @GlauberF @googlebot @mmalerba @andrewseguin