-
Notifications
You must be signed in to change notification settings - Fork 766
Add multi-column sorting with stack-based behavior #6257
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
Merged
Merged
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
fb86632
Add multi-column sorting with stack-based behavior
lucharo fdbad05
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 1c8e68c
Remove Claude settings file
lucharo 03f13ed
Fix multi-column sorting RPC serialization
lucharo 00cac12
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 4038124
Add multi-column sorting with stack-based priority
lucharo 819c912
Add tests for multi-column sorting logic
lucharo b567353
Merge remote-tracking branch 'origin/main' into multi-column-sort
lucharo 2f95bd1
Merge remote-tracking branch 'upstream/main' into multi-column-sort
lucharo ac9c7f1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 9022d36
Use list[SortArgs] instead of list[tuple] for cleaner API
lucharo cc78a02
Simplify sort_values logic in DefaultTableManager
lucharo a88a12d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] b42c4fc
Refactor: reduce duplication in sort UI and data conversion
lucharo 12cb0d2
Address Light2Dark's PR review feedback
lucharo 6a6f2e1
Fix linting issues
lucharo 4dff80d
Fix circular import with TYPE_CHECKING
lucharo 7deb726
Add missing SortDirection type import
lucharo b3840f5
Remove unnecessary tuple conversion in dataframe.py
lucharo f9dfab3
Update all tests to use SortArgs instead of tuples
lucharo 573f6a6
Fix mixed-type and None sorting in DefaultTableManager
lucharo afde587
Address PR feedback: filter invalid sort columns and remove artifacts
lucharo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
117 changes: 117 additions & 0 deletions
117
frontend/src/components/data-table/__tests__/header-items.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| /* Copyright 2024 Marimo. All rights reserved. */ | ||
|
|
||
| import type { SortingState } from "@tanstack/react-table"; | ||
| import { describe, expect, it, vi } from "vitest"; | ||
|
|
||
| describe("multi-column sorting logic", () => { | ||
| // Extract the core sorting logic to test in isolation | ||
| const handleSort = ( | ||
| columnId: string, | ||
| desc: boolean, | ||
| sortingState: SortingState, | ||
| setSorting: (state: SortingState) => void, | ||
| clearSorting: () => void, | ||
| ) => { | ||
| const currentSort = sortingState.find((s) => s.id === columnId); | ||
|
|
||
| if (currentSort && currentSort.desc === desc) { | ||
| // Clicking the same sort again - remove it | ||
| clearSorting(); | ||
| } else { | ||
| // New sort or different direction - move to end of stack | ||
| const otherSorts = sortingState.filter((s) => s.id !== columnId); | ||
| const newSort = { id: columnId, desc }; | ||
| setSorting([...otherSorts, newSort]); | ||
| } | ||
| }; | ||
|
|
||
| it("implements stack-based sorting: moves re-clicked column to end", () => { | ||
| const sortingState: SortingState = [ | ||
| { id: "name", desc: false }, | ||
| { id: "age", desc: false }, | ||
| ]; | ||
| const setSorting = vi.fn(); | ||
| const clearSorting = vi.fn(); | ||
|
|
||
| // Click Desc on age - should move age to end with desc=true | ||
| handleSort("age", true, sortingState, setSorting, clearSorting); | ||
|
|
||
| expect(setSorting).toHaveBeenCalledWith([ | ||
| { id: "name", desc: false }, | ||
| { id: "age", desc: true }, | ||
| ]); | ||
| expect(clearSorting).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("removes sort when clicking same direction twice", () => { | ||
| const sortingState: SortingState = [{ id: "age", desc: false }]; | ||
| const setSorting = vi.fn(); | ||
| const clearSorting = vi.fn(); | ||
|
|
||
| // Click Asc on age again - should remove the sort | ||
| handleSort("age", false, sortingState, setSorting, clearSorting); | ||
|
|
||
| expect(clearSorting).toHaveBeenCalled(); | ||
| expect(setSorting).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("adds new column to end of stack", () => { | ||
| const sortingState: SortingState = [{ id: "name", desc: false }]; | ||
| const setSorting = vi.fn(); | ||
| const clearSorting = vi.fn(); | ||
|
|
||
| // Click Asc on age - should add age to end | ||
| handleSort("age", false, sortingState, setSorting, clearSorting); | ||
|
|
||
| expect(setSorting).toHaveBeenCalledWith([ | ||
| { id: "name", desc: false }, | ||
| { id: "age", desc: false }, | ||
| ]); | ||
| expect(clearSorting).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("toggles sort direction when clicking opposite", () => { | ||
| const sortingState: SortingState = [{ id: "age", desc: false }]; | ||
| const setSorting = vi.fn(); | ||
| const clearSorting = vi.fn(); | ||
|
|
||
| // Click Desc on age - should toggle to descending | ||
| handleSort("age", true, sortingState, setSorting, clearSorting); | ||
|
|
||
| expect(setSorting).toHaveBeenCalledWith([{ id: "age", desc: true }]); | ||
| expect(clearSorting).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("correctly calculates priority numbers", () => { | ||
| const sortingState: SortingState = [ | ||
| { id: "name", desc: false }, | ||
| { id: "age", desc: true }, | ||
| { id: "dept", desc: false }, | ||
| ]; | ||
|
|
||
| // Priority is index + 1 | ||
| const nameSort = sortingState.find((s) => s.id === "name"); | ||
| const namePriority = nameSort ? sortingState.indexOf(nameSort) + 1 : null; | ||
| expect(namePriority).toBe(1); | ||
|
|
||
| const deptSort = sortingState.find((s) => s.id === "dept"); | ||
| const deptPriority = deptSort ? sortingState.indexOf(deptSort) + 1 : null; | ||
| expect(deptPriority).toBe(3); | ||
| }); | ||
|
|
||
| it("handles removing column from middle of stack", () => { | ||
| const sortingState: SortingState = [ | ||
| { id: "name", desc: false }, | ||
| { id: "age", desc: true }, | ||
| { id: "dept", desc: false }, | ||
| ]; | ||
| const setSorting = vi.fn(); | ||
| const clearSorting = vi.fn(); | ||
|
|
||
| // Click Desc on age again - should remove it | ||
| handleSort("age", true, sortingState, setSorting, clearSorting); | ||
|
|
||
| expect(clearSorting).toHaveBeenCalled(); | ||
| // After removal, dept should move from priority 3 to priority 2 | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hey Lucharo, I think this will be a nicer refactor, you can use the column.toggleSorting to achieve what you want instead of manually setting the table sort