Skip to content

feat(table): add text column for simple columns #14841

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

Merged
merged 8 commits into from
Apr 23, 2019

Conversation

andrewseguin
Copy link
Contributor

@andrewseguin andrewseguin commented Jan 15, 2019

Adds an easier way to add columns to the table for cases where the cells are simply text values.

Rather than writing this:

<ng-container matColumnDef="score">
  <th mat-header-cell *matHeaderCellDef> Score </th>
  <td mat-cell *matCellDef="let user"> {{user.score}} </td>
</ng-container>

you can instead write:

<mat-text-column name="score"></mat-text-column>

For simple cases, and for users that just want to quickly bring up a table and iterate, this is much easier to write and remember.

Considerations: Do we need to add a mat-number-column? The only difference will be that it would default to justify="end".

Fixes #9925

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 15, 2019
@andrewseguin andrewseguin added the target: minor This PR is targeted for the next minor release label Jan 22, 2019
* Column that simply shows text content for the header and row cells. Assumes that the table
* is using the native table implementation (`<table>`).
*
* By default, the name of this column will be the header text and data property accessor.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should default to using the column name for property access because it's just setting people up for production bugs when the property name is minified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to lean towards leaving the behavior as is, but make sure to mention in the documentation that this may come up. I suspect it is very rare for users to have minified properties on their table data and the added API requirement will seem very redundant for everyone else.

We do something similar in the MatTableDataSource where, by default, the sort accessor (sortingDataAccessor) uses property access by the sort header ID which will have the same minification problems. I haven't seen any issues come in regarding confusion about it.

Happy to chat more about it in case I misunderstand how often this would occur

Copy link
Member

Choose a reason for hiding this comment

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

Okay- can you add the documentation in this PR?

}

if (this.table) {
this.table.addColumnDef(this.columnDef);
Copy link
Member

Choose a reason for hiding this comment

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

Should there be an error if there's no table instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - I can't think of a good reason why it'd be useful otherwise.

@jelbourn
Copy link
Member

jelbourn commented Apr 2, 2019

Overall looks good aside from documentation for that property renaming concern

@andrewseguin
Copy link
Contributor Author

Added noted saying you should use the data accessor if your data may be minified - feel free to suggest other wording

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

@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Apr 9, 2019
@andrewseguin andrewseguin added the P2 The issue is important to a large percentage of users, with a workaround label Apr 23, 2019
@mmalerba mmalerba merged commit cf76707 into angular:master Apr 23, 2019
RudolfFrederiksen pushed a commit to RudolfFrederiksen/material2 that referenced this pull request Jun 21, 2019
* feat(table): add text column for simple columns

* minor review changes

* fix lint

* add note regarding minification

* add static true to query for column def

* change cell query to static true

* revert static query in cell; directly provide cells to column

* new goldens
@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 10, 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 P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants