- 
                Notifications
    You must be signed in to change notification settings 
- Fork 749
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 21 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
  
    
      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,3 @@ | ||
| { | ||
| "outputStyle": "friendly-educational" | ||
| } | ||
|         
                  lucharo marked this conversation as resolved.
              Outdated
          
            Show resolved
            Hide resolved | 
              Empty file.
          
    
        
          
  
    
      
          
            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.
  
    
  
    
Uh oh!
There was an error while loading. Please reload this page.