-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Conversation
ca14529
to
697c633
Compare
* 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
24b3e9c
to
c94ce33
Compare
Overall looks good aside from documentation for that property renaming concern |
c94ce33
to
9dd6a46
Compare
Added noted saying you should use the data accessor if your data may be minified - feel free to suggest other wording |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
9dd6a46
to
d26e130
Compare
94a8531
to
c5fe890
Compare
5279faf
to
3bd81a3
Compare
* 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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds an easier way to add columns to the table for cases where the cells are simply text values.
Rather than writing this:
you can instead write:
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 tojustify="end"
.Fixes #9925