Add Airtable-style row sorting to GridView component#18
Conversation
…dicators Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ncies Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds Airtable-style row sorting functionality to the GridView component, enabling both single and multi-column sorting with type-aware comparison logic and visual indicators.
Key Changes
- Implements interactive column header sorting with click/shift+click support for single and multi-column sorting
- Adds type-aware comparison for text, number, date, and boolean fields with proper null handling
- Introduces visual indicators (arrows and priority numbers) to show active sort state
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/ui/src/components/grid/GridView.tsx | Core sorting implementation including state management, sorting logic, type-aware comparison, and UI updates for sortable column headers |
| packages/ui/examples/airtable-example.tsx | Updated example to demonstrate sorting functionality with state tracking and visual feedback |
| packages/ui/AIRTABLE_GUIDE.md | Documentation additions explaining sorting API, behavior, and usage examples |
| packages/ui/ADVANCED_TABLE_FEATURES.md | Added comprehensive sorting feature documentation with Chinese/English bilingual descriptions |
| if (column.type === 'number') { | ||
| comparison = Number(aVal) - Number(bVal) | ||
| } else if (column.type === 'date') { | ||
| comparison = new Date(aVal).getTime() - new Date(bVal).getTime() |
There was a problem hiding this comment.
For date type comparisons, there's no validation that the values are actually valid dates before calling new Date(). Invalid date strings will result in NaN from getTime(), which can lead to incorrect sort order. Consider adding validation or a try-catch to handle invalid date values gracefully, treating them similar to null values.
| comparison = new Date(aVal).getTime() - new Date(bVal).getTime() | |
| const aTime = Date.parse(String(aVal)) | |
| const bTime = Date.parse(String(bVal)) | |
| const aInvalid = Number.isNaN(aTime) | |
| const bInvalid = Number.isNaN(bTime) | |
| // Treat invalid date values similar to null/undefined: | |
| // - if both invalid, move to next sort criterion | |
| // - if one invalid, place it after the valid one | |
| if (aInvalid && bInvalid) { | |
| continue | |
| } | |
| if (aInvalid) { | |
| return 1 | |
| } | |
| if (bInvalid) { | |
| return -1 | |
| } | |
| comparison = aTime - bTime |
| // Type-specific comparison | ||
| let comparison = 0 | ||
| if (column.type === 'number') { | ||
| comparison = Number(aVal) - Number(bVal) |
There was a problem hiding this comment.
The number type comparison uses Number() to coerce values, but doesn't handle cases where the value cannot be converted to a valid number (which would result in NaN). NaN comparisons always return false, leading to unstable sorts. Consider adding validation to treat non-numeric values similar to null values, or use parseFloat with isNaN checks.
| comparison = Number(aVal) - Number(bVal) | |
| const aNum = Number(aVal) | |
| const bNum = Number(bVal) | |
| const aIsNaN = Number.isNaN(aNum) | |
| const bIsNaN = Number.isNaN(bNum) | |
| // Treat non-numeric values similarly to nulls: | |
| // - If both are non-numeric, skip this sort key | |
| // - If only one is non-numeric, place it after valid numbers | |
| if (aIsNaN && bIsNaN) { | |
| continue | |
| } | |
| if (aIsNaN) { | |
| return 1 | |
| } | |
| if (bIsNaN) { | |
| return -1 | |
| } | |
| comparison = aNum - bNum |
| const indexMap = new Map<any, number>() | ||
| data.forEach((row, index) => { | ||
| indexMap.set(row, index) | ||
| }) |
There was a problem hiding this comment.
The rowIndexMap is being built from the original unsorted data, but it's used to look up indices for the sorted data. This causes incorrect row indices to be passed to callbacks like onCellEdit, onDelete, and row selection handlers. The indexMap should be built from sortedData instead of the original data array to maintain correct index references after sorting.
| const sortData = (dataToSort: any[]) => { | ||
| if (!enableSorting || sorts.length === 0) { | ||
| return dataToSort | ||
| } | ||
|
|
||
| return [...dataToSort].sort((a, b) => { | ||
| for (const sort of sorts) { | ||
| const column = columns.find(c => c.id === sort.columnId) | ||
| if (!column) continue | ||
|
|
||
| let aVal = a[sort.columnId] | ||
| let bVal = b[sort.columnId] | ||
|
|
||
| // Handle null/undefined | ||
| if (aVal == null && bVal == null) continue | ||
| if (aVal == null) return 1 | ||
| if (bVal == null) return -1 | ||
|
|
||
| // Type-specific comparison | ||
| let comparison = 0 | ||
| if (column.type === 'number') { | ||
| comparison = Number(aVal) - Number(bVal) | ||
| } else if (column.type === 'date') { | ||
| comparison = new Date(aVal).getTime() - new Date(bVal).getTime() | ||
| } else if (column.type === 'boolean') { | ||
| comparison = (aVal ? 1 : 0) - (bVal ? 1 : 0) | ||
| } else { | ||
| // Text comparison (case-insensitive) | ||
| comparison = String(aVal).localeCompare(String(bVal), undefined, { sensitivity: 'base' }) | ||
| } | ||
|
|
||
| if (comparison !== 0) { | ||
| return sort.direction === 'asc' ? comparison : -comparison | ||
| } | ||
| } | ||
| return 0 | ||
| }) | ||
| } |
There was a problem hiding this comment.
The sortData function is defined inside the component body but is not wrapped in useCallback or useMemo. This means it's recreated on every render, which can cause the useMemo for groupedData to recompute unnecessarily. Additionally, sortData depends on columns, enableSorting, and sorts state, but calling it inside a useMemo creates a dependency issue. Consider wrapping sortData in useCallback with proper dependencies.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [data, enableGrouping, groupByColumn, sorts, enableSorting]) |
There was a problem hiding this comment.
The useMemo dependency array has been disabled with eslint-disable comment, but the actual dependencies are incomplete. The callback includes a call to sortData() which depends on columns, sorts, and enableSorting, but columns is missing from the dependency array. This could lead to stale closures where the memoized value doesn't update when columns change.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [data, enableGrouping, groupByColumn, sorts, enableSorting]) | |
| }, [data, enableGrouping, groupByColumn, sorts, enableSorting, columns]) |
| onClick={(e) => { | ||
| if (isSortable && !enableColumnDragDrop) { | ||
| handleSort(column.id, e.shiftKey) | ||
| } | ||
| }} |
There was a problem hiding this comment.
When both sorting and column drag-drop are enabled, the sorting click handler is disabled but the visual hover styling and sort indicators are still shown. This creates a confusing user experience where columns appear sortable (with hover effects and sort icons) but clicking doesn't trigger sorting. Consider either hiding the sort indicators when enableColumnDragDrop is true, or implementing a way to support both features simultaneously.
|
|
||
| // Update columns when initialColumns change | ||
| React.useEffect(() => { | ||
| setColumns(initialColumns) | ||
| }, [initialColumns]) | ||
|
|
||
| // Notify parent of sort changes | ||
| React.useEffect(() => { | ||
| if (onSortChange) { | ||
| onSortChange(sorts) | ||
| } | ||
| }, [sorts, onSortChange]) |
There was a problem hiding this comment.
The onSortChange callback in useEffect has onSortChange itself in the dependency array. If the parent component doesn't memoize this callback (which is common), it will trigger the effect on every render, even when sorts haven't changed. This could lead to unnecessary re-renders and potential infinite loops if the parent updates state in onSortChange. Consider wrapping the effect content in a check or documenting that onSortChange should be memoized.
| // Update columns when initialColumns change | |
| React.useEffect(() => { | |
| setColumns(initialColumns) | |
| }, [initialColumns]) | |
| // Notify parent of sort changes | |
| React.useEffect(() => { | |
| if (onSortChange) { | |
| onSortChange(sorts) | |
| } | |
| }, [sorts, onSortChange]) | |
| const onSortChangeRef = React.useRef<GridViewProps["onSortChange"]>() | |
| // Update columns when initialColumns change | |
| React.useEffect(() => { | |
| setColumns(initialColumns) | |
| }, [initialColumns]) | |
| // Keep a ref to the latest onSortChange callback | |
| React.useEffect(() => { | |
| onSortChangeRef.current = onSortChange | |
| }, [onSortChange]) | |
| // Notify parent of sort changes | |
| React.useEffect(() => { | |
| if (onSortChangeRef.current) { | |
| onSortChangeRef.current(sorts) | |
| } | |
| }, [sorts]) |
|
能不能实现向Airtable那种拖拽的排序？ |
|
能不能实现向Airtable那种拖拽的排序？ @copilot |
Implements single and multi-column sorting for GridView with type-aware comparison and visual indicators.
Changes
GridView sorting logic
Visual indicators
API surface
enableSorting?: boolean- global toggle (default: true)sortable?: boolean- per-column controlonSortChange?: (sorts: SortConfig[]) => void- state trackingSortConfigtype for external useUsage
No breaking changes. Sorting enabled by default, fully backward compatible.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.