feat(#3344): add multi-column sorting to Table and TableSortHeader#3409
feat(#3344): add multi-column sorting to Table and TableSortHeader#3409
Conversation
c1514d7 to
7097784
Compare
bdfranck
left a comment
There was a problem hiding this comment.
The sorting interactions are looking good! I've left some comments that can be grouped into three categories:
- Try to re-use and expand on existing variables and functions.
- Avoid using JSON because it can lead to increased errors. Use Types instead.
- Make sure that you're giving devs the data they need for their sort functions.
| /** Sort mode: "single" allows one column, "multi" allows up to 2 columns. */ | ||
| export let sortmode: SortMode = "single"; | ||
| /** Initial sort configuration as JSON string: [{"column":"name","direction":"asc"}] */ | ||
| export let initialsort: string = ""; |
There was a problem hiding this comment.
Using a JSON string here could introduce errors. We can take advantage of TypeScript's error prevention by using an array of SortEntries instead. Then we can remove _sorts as it will be redundant.
The checkbox list is a good reference. It uses an array of strings for keeping track of multiple selected values.
There was a problem hiding this comment.
I agree vs the point that we shouldn' use json.
I think instead of using initialsort as a JSON string on the table (which is a new property), We could declare the initial sort state directly on each goa-table-sort-header (which we do nowadays)
For example:
<goa-table sortmode="multi">
... <goa-table-sort-header name="name" direction="asc" sortorder="1">Name</goa-table-sort-header>
... <goa-table-sort-header name="department" direction="desc" sortorder="2">Department</goa-table-sort-header>
...<goa-table-sort-header name="salary">Salary</goa-table-sort-header>
</goa-table>
So we only add a new property sortorder to the goa-table-sort-header. This approach will be consistent with how the old single-sort already worked. The table just scans headers onMount to build its internal sort state from direction + sortorder attribute.
| } | ||
| } | ||
|
|
||
| function initializeSorts() { |
There was a problem hiding this comment.
You can remove this function when you switch to using an array of SortEntries for the sorts.
There was a problem hiding this comment.
If we rely on GoabTableSortHeader then refer this comment #3409 (comment), we can keep this function to "scan" header and then initialize the table sort state.
| import type { Spacing } from "../../common/styling"; | ||
|
|
||
| // Types | ||
| type SortDirection = "asc" | "desc"; |
There was a problem hiding this comment.
There's an existing type that you can use. It's already imported into this component:
| /** @deprecated Use onSortChange for new implementations */ | ||
| onSort?: (detail: GoabTableOnSortDetail) => void; | ||
| /** Called when sort state changes. Receives array of current sorts. */ | ||
| onSortChange?: (sorts: GoabTableSortEntry[]) => void; |
There was a problem hiding this comment.
Instead of building a parallel function that does the same thing as onSort, why not expand onSort to support either a single item or an array?
GoabTableOnSortDetail[] | GoabTableOnSortDetail
This would allow you to add new functionality while preserving the original structure.
|
|
||
| // New API: onSortChange receives sorts array | ||
| if (onSortChange && "sorts" in detail) { | ||
| onSortChange(detail.sorts); |
There was a problem hiding this comment.
I noticed that this will send a "asc" or "desc" string for direction instead of 1 or -1. Is this intentional? Devs will need a number for their sort function like we have in our example:
https://design.alberta.ca/components/table#tab-1#table-with-sortable-columns
| @Input() initialSort?: GoabTableSortEntry[]; | ||
| @Input({ transform: booleanAttribute }) striped?: boolean; | ||
|
|
||
| get initialSortJson(): string | undefined { |
There was a problem hiding this comment.
Let's avoid using JSON here too. The initial sort should be an array to reduce errors.
| .direction--none goa-icon { | ||
| height: 0.625rem; | ||
| /* Sort order number badge */ | ||
| .sort-order { |
There was a problem hiding this comment.
I tested and see there is a small shift in a column:
https://jam.dev/c/8bafde76-cd5d-4d17-8e1e-68912f31a9b1
I think we should add a hidden class for this .sort-order, for example:
<span class='sort-order' class:hidden={!sortorder || direction === 'none'>
Then we can make sure width:0 and margin:0 to prevent the shifting.
libs/web-components/src/components/table/TableSortHeader.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/table/TableSortHeader.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/table/TableSortHeader.svelte
Outdated
Show resolved
Hide resolved
7097784 to
48f879a
Compare
❌ Deploy Preview for goa-design-2 failed.
|
- Add sortMode prop ("single" | "multi") to Table for multi-column sorting
- Add initialSort prop to pre-configure sort state
- Add onSortChange callback returning array of current sorts
- Add sortOrder prop to TableSortHeader for priority display ("1", "2")
- Table manages sort state internally, updates headers automatically
- Change sort icons: chevron-expand (unsorted), arrow-up/down (sorted)
- Add V2 focus styling (ring on icon only)
- Update React and Angular wrappers (lib + experimental)
- Add test pages for React and Angular playgrounds
Closes #3344
48f879a to
53750ae
Compare
Summary
Adds built-in multi-column sorting to Table and TableSortHeader components.
"single"(default) or"multi"(up to 2 columns)chevron-expand(unsorted),arrow-up/arrow-down(sorted)Multi sort
Single sort
Test plan
/features/3344/features/3344and repeat testsCloses #3344