-
Notifications
You must be signed in to change notification settings - Fork 63
feat: Enable multi-column sorting in anywidget mode #2360
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 46 commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
218b7c2
feat: Enhance Series and DataFrame display with anywidget
shuoweil 9e3163f
test: add more npm tests
shuoweil 3227b23
test: add this file for faster and reliable npm tests
shuoweil c1a8f83
docs: notebook update
shuoweil c39293a
test: update old testcase due to new feature implementation
shuoweil 9dff0f4
Revert "test: update old testcase due to new feature implementation"
shuoweil f67e30f
feat: only display row count when series is large than the number can…
shuoweil 81d1dbe
refactor: Handle special float values and None consistently in sqlglo…
chelsea-lin 057f54d
Merge branch 'main' into shuowei-anywidget-series-display
shuoweil f70d5c1
Merge branch 'main' into shuowei-anywidget-series-display
shuoweil 58e357a
Merge branch 'main' into shuowei-anywidget-series-display
shuoweil fd04e6a
refactor: code refactor
shuoweil 4825aeb
fix: fix mypy
shuoweil 8845464
Merge branch 'main' into shuowei-anywidget-series-display
shuoweil d36fc0f
refactor: move code to plaintext file and add checks
shuoweil 593f9ae
refactor: move code to plaintext file and add checks
shuoweil 400ea07
Revert "refactor: move code to plaintext file and add checks"
shuoweil 945616c
Merge branch 'main' into shuowei-anywidget-series-display
shuoweil a474606
refactor: move create_text_representation to plaintext.py
shuoweil bd56992
refactor: move display logic to display/plaintext.py and display/html.py
shuoweil 971ee33
refactor: restore original order of max_results in __repr__
shuoweil 1a73628
docs: add todo back
shuoweil 1b7952b
refactor: split repr_mimebundle logic, handle deferred mode in html, …
shuoweil 15b2ac6
refactor: rename repr_mimebundle helpers and improve fallback comments
shuoweil f8914c8
style: fix repr_mimebundle docstring formatting
shuoweil 9fea10e
docs: update anywidget demo notebook with series display showcase
shuoweil a20a5ee
docs: update notebook
shuoweil 9e92c2a
Merge branch 'main' into shuowei-anywidget-series-display
shuoweil c0f4b4e
refactor: decouple plaintext representation from core objects
shuoweil 38899b7
refactor: consolidate object metadata extraction for display
shuoweil 64230d1
fix: refactor html display to address review comments
shuoweil 2140b0f
feat: Refactor DataFrame.to_pandas_batches and update TableWidget
shuoweil 15eb28d
test: fix failed testcase
shuoweil 95ed29c
feat: remove unsafe while loop
shuoweil fcb6700
chore: document why RLock is needed in TableWidget
shuoweil a42506c
fix(display): prevent deadlock in TableWidget page update
shuoweil 3eed5fc
fix: make TableWidget import-safe when traitlets is missing
shuoweil 69a3272
fix: refactor test_anywidget imports to avoid NameError when dependen…
shuoweil a9de237
test(unit): refine TableWidget deadlock test style
shuoweil f7ad44e
test: skip windows test
shuoweil e016751
Merge branch 'main' into shuowei-anywidget-sort-multi-column
shuoweil 50bee9c
feat(display): support multi-column sorting in anywidget
shuoweil e98edb2
docs: update anywidget_mode notebook with multi-column sorting instru…
shuoweil 2a421ca
docs: rerun the notebook for testing
shuoweil 70d1659
test(js): add multi-column sorting test case and update existing tests
shuoweil b5306c1
test: fix failed tests
shuoweil b755c55
refactor(anywidget): merge sort_columns/ascending into sort_context
shuoweil 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
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.
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.
I suspect that this data structure allows content that make little sense in practice. For example:
Does it make sense to merge these two fields into a dictionary (ordered map), which maps the column name to whether it's ascending?
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.
Thanks for the suggestion! I’ve refactored the two fields into a single sort_context traitlet to ensure atomic updates and maintain a consistent data state.
Regarding the concern about practical validity: the frontend logic is designed to ensure the list remains sane. When a user clicks a column header, we verify if the column is orderable and handle the list logic so that the same column cannot be inserted multiple times (it instead cycles through states or is removed).
While the original design was safe within these UI constraints, I agree that merging them is a cleaner architectural choice. I chose a List of Dictionaries (e.g., {'column': 'col1', 'ascending': True}]) instead of a Map for a few reasons:
The result is verified at here: screen/BxN5ZX3QTG6TEU8 (the description box is updated accordingly)
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.
Thanks!