-
Notifications
You must be signed in to change notification settings - Fork 20
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
Table Column Sorting #210
Table Column Sorting #210
Conversation
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.
Left some comments, I think maybe we can simplify this a little bit.
sortConfig?: GoTableSortConfig, | ||
tableData: any[] | ||
} = this.localTableConfig; | ||
|
||
if (tableData && sortable) { | ||
if (tableData) { |
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 think we want to keep this because this will restrict sorting on a table level, not a column level.
@@ -71,6 +72,14 @@ export class GoTableComponent implements OnInit, OnChanges { | |||
this.renderTable(); | |||
} | |||
|
|||
ngAfterContentInit(): void { |
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.
If we default sorting to true
on a column, then we won't have to worry about any of this.
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.
@grahamhency We can default sorting to true, but not defaulting it allows us to have a little more flexibility in how the table is implemented. This way instead of just turning sorting off for a couple columns, we can turn off sorting for the entire table and then enable sorting on a couple columns. The only way to do this is to always keep track of sorting on the column instead of the table, but if this isn't something we care about theres a few things I can clean up here that wouldn't be necessary.
@StevenUlmer did you want to have a discussion about the comments I left on this? Also, we'll want to add documentation for this. |
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.
@StevenUlmer what if we pass the sortable
property of the table into the column and use that as the default for the column?
@Input tableSortable: boolean;
@Input sortable: boolean;
ngOnInit(): {
if (this.sortable === undefined) {
this.sortable = this.tableSortable;
}
}
38cf580
to
3370e0b
Compare
@grahamhency I've removed that afterContentInit by changing some of the html. Can you take another look? |
3370e0b
to
4f43560
Compare
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.
This functionality looks good. Ready for some documentation.
Currently we have the ability to either have every column on a table sortable or none of them. There are certain cases where we want to be able to just sort by a couple columns on a table. This provides a way for an implementor to decide which columns can be sortable on a table or which columns they want to not be sortable.
4f43560
to
1dc287c
Compare
@grahamhency Documentation has been written |
Merged, thanks for writing the docs! |
closes #180
Currently we have the ability to either have every column on a table sortable or none of them. There
are certain cases where we want to be able to just sort by a couple columns on a table. This provides a way for an implementor to decide which columns can be sortable on a table or which columns they want to not be sortable. If a column doesn't contain the sortable binding it will default to whatever is in the table config.