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

feat(table): add row when predicate #6795

Merged
merged 1 commit into from
Oct 3, 2017

Conversation

andrewseguin
Copy link
Contributor

Fixes #5887

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 1, 2017
@andrewseguin andrewseguin added pr: needs review and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Sep 1, 2017
_getRowDef(data: T, i: number): CdkRowDef<T> {
if (this._rowDefs.length == 1) { return this._rowDefs.first; }

let rowDef = this._rowDefs.find(def => def.when && def.when(data, i)) || this._defaultRowDef;
Copy link
Member

Choose a reason for hiding this comment

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

Could just default when to () => false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this mean everyone would need to include a when function when defining their rows? I figured we wanted to let them leave it off for a row and it'll be the default

Copy link
Member

Choose a reason for hiding this comment

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

In that case would defaulting to () => true do the same thing? The first row with no when will be the default, all other rows with no when will be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's fine with me. I think I was trying to catch the user's error ahead of time if they didn't provide a default. But it's not a perfect thing to catch anyways (all rows could have when but still catch 100% of cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Righto - I remember why its needed. If we default to true, that row is already our match when we look for a row def. If we default to () => false then it forces users to always define a when function

@@ -7,17 +7,17 @@ import {MdTableModule} from './index';
import {MdTable} from './table';

describe('MdTable', () => {
let fixture: ComponentFixture<SimpleMdTableApp>;
let fixture: ComponentFixture<MdTableApp>;
Copy link
Member

Choose a reason for hiding this comment

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

  • Add test case for when missing on multiple rows?
  • Add test case for using the default row def?

There's also no test now for a table that just has one row def

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test for missing when on multiple rows. Default row def is used for any case where there is no when, e.g. SimpleCdkTableApp

Note, majority of testing for the when logic is in cdk/table.ts. Goal of the lib/table.ts test is to show that the MdRowDef successfully reads the when input and passes it on to the CdkRowDef.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 5, 2017
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

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Sep 5, 2017
@kara kara merged commit 0875b85 into angular:master Oct 3, 2017
@andrewseguin andrewseguin deleted the table-when-predicate branch November 28, 2017 20:40
@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 Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Table] Apply conditional rows using by a when-predicate
4 participants