-
Notifications
You must be signed in to change notification settings - Fork 21
feat: implement sortable columns with visual indicators and sorting l… #414
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
base: next
Are you sure you want to change the base?
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.
Pull Request Overview
This PR adds column sorting functionality to the Table component, supporting both client-side and server-side sorting modes.
- Adds interactive column headers with visual sort indicators (ascending/descending/unsorted)
- Implements client-side sorting for array data sources with support for nested field paths, dates, and numbers
- Extends server-side data fetching to pass sort parameters to the data provider function
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adminforth/spa/src/afcl/Table.vue
Outdated
| <!-- Unsorted indicator --> | ||
| <svg v-if="!isSorted(column)" class="w-3 h-3 ms-1.5 opacity-30" aria-hidden="true" xmlns="http://www.w3.org/2000/svg" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 1.086Zm6.852 1.952H8.574a2.072 2.072 0 0 0-1.847 1.087 1.9 1.9 0 0 0 .11 1.985l3.426 5.05a2.123 2.123 0 0 0 3.472 0l3.427-5.05a1.9 1.9 0 0 0 .11-1.985 2.074 2.074 0 0 0-1.846-1.087Z"></path></svg> | ||
| <!-- Sorted ascending indicator --> | ||
| <svg v-else-if="currentSortDirection === 'asc'" class="w-3 h-3 ms-1.5" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 0z"></path></svg> |
Copilot
AI
Nov 4, 2025
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.
The SVG path for ascending sort indicator is incomplete. The path ends with '1.847 0z' but should end with '1.086Zm6.852...' to match the complete path from the unsorted indicator. This will result in an incorrect visual representation of the ascending arrow.
| <svg v-else-if="currentSortDirection === 'asc'" class="w-3 h-3 ms-1.5" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 0z"></path></svg> | |
| <svg v-else-if="currentSortDirection === 'asc'" class="w-3 h-3 ms-1.5" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 1.086Z"></path></svg> |
adminforth/spa/src/afcl/Table.vue
Outdated
| <!-- Sorted ascending indicator --> | ||
| <svg v-else-if="currentSortDirection === 'asc'" class="w-3 h-3 ms-1.5" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 0z"></path></svg> | ||
| <!-- Sorted descending indicator (rotated) --> | ||
| <svg v-else class="w-3 h-3 ms-1.5 rotate-180" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 0z"></path></svg> |
Copilot
AI
Nov 4, 2025
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.
The SVG path for descending sort indicator has the same incomplete path issue as the ascending indicator. The path ends with '1.847 0z' but should include the complete path definition to properly render the arrow icon.
| <svg v-else class="w-3 h-3 ms-1.5 rotate-180" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 0z"></path></svg> | |
| <svg v-else class="w-3 h-3 ms-1.5 rotate-180" fill="currentColor" viewBox="0 0 24 24"><path d="M8.574 11.024h6.852a2.075 2.075 0 0 0 1.847-1.086 1.9 1.9 0 0 0-.11-1.986L13.736 2.9a2.122 2.122 0 0 0-3.472 0L6.837 7.952a1.9 1.9 0 0 0-.11 1.986 2.074 2.074 0 0 0 1.847 1.086Zm6.852 1.952H8.574a2.072 2.072 0 0 0-1.847 1.087 1.9 1.9 0 0 0 .11 1.985l3.426 5.05a2.123 2.123 0 0 0 3.472 0l3.427-5.05a1.9 1.9 0 0 0 .11-1.985 2.074 2.074 0 0 0-1.846-1.087Z"></path></svg> |
| emit('update:sortField', currentSortField.value); | ||
| emit('update:sortDirection', currentSortField.value ? currentSortDirection.value : undefined as any); | ||
| emit('sort-change', { field: currentSortField.value, direction: currentSortDirection.value }); | ||
| }); |
Copilot
AI
Nov 4, 2025
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.
The watcher triggers on initial component mount when currentSortField is initialized with props.defaultSortField, causing an unnecessary refresh() call after the onMounted hook already calls refresh(). Consider using { immediate: false } option or checking if this is the initial load to avoid duplicate data fetching.
| }); | |
| }, { immediate: false }); |
adminforth/spa/src/afcl/Table.vue
Outdated
| if (currentPage.value !== 1) currentPage.value = 1; | ||
| refresh(); | ||
| emit('update:sortField', currentSortField.value); | ||
| emit('update:sortDirection', currentSortField.value ? currentSortDirection.value : undefined as any); |
Copilot
AI
Nov 4, 2025
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.
The as any type assertion bypasses TypeScript's type checking. Consider properly typing the emit parameter or using a union type like 'asc' | 'desc' | undefined in the emit definition instead of forcing the type with as any.
| emit('update:sortDirection', currentSortField.value ? currentSortDirection.value : undefined as any); | |
| emit('update:sortDirection', currentSortField.value ? currentSortDirection.value : undefined); |
adminforth/spa/src/afcl/Table.vue
Outdated
| offset: (currentLoadingIndex - 1) * props.pageSize, | ||
| limit: props.pageSize, | ||
| sortField: currentSortField.value, | ||
| sortDirection: currentSortDirection.value, |
Copilot
AI
Nov 4, 2025
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.
When sorting is cleared (sortField is undefined), the function still passes currentSortDirection.value which will be 'asc' by default. This could confuse server-side implementations. Consider only passing sortDirection when sortField has a value, e.g., ...(currentSortField.value ? { sortDirection: currentSortDirection.value } : {}).
| sortDirection: currentSortDirection.value, | |
| ...(currentSortField.value ? { sortDirection: currentSortDirection.value } : {}), |
…ogic