-
Notifications
You must be signed in to change notification settings - Fork 126
Notebooks: Track the Active Cell For Selection #10573
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: main
Are you sure you want to change the base?
Conversation
|
E2E Tests 🚀 |
The active state of a cell is orthogonal to its selectionStatus. A cell can be active and have a selection status of editing or selected. We need to track both states separately. The isActive property on the cell is added out of convenience so the getActiveCell helper function doesn't need to be used in components that already have access to the cell object.
e4702ee to
cb1e40b
Compare
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.
Removed the onMenuStateChange prop because its not needed in NotebookCellActionBar.tsx anymore.
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.
A good chunk of the styles in here that don't have a comment about being for hover behavior were changes needed so the focus border shows up on the active cell.
78b652c to
266c325
Compare
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.
Epic PR! 🎉 Feels much nicer and smoother when growing/shrinking the selection, escaping out of a multi selection, and hovering over cells
Separate but related bug: Shift+Enter should execute all selected cells in the order they appear (not order they're selected), but currently executes only the active cell.
Screen.Recording.2025-11-17.at.14.03.23.mov
Super minor nit for discussion: Something about how the gap between the cell code bar and output bar fits into the cell border doesn't feel right:
|
|
||
| public readonly executionStatus; | ||
| public readonly selectionStatus = observableValue<CellSelectionStatus, void>('cellSelectionStatus', CellSelectionStatus.Unselected); | ||
| public readonly isActive = observableValue<boolean, void>('cellIsActive', false); |
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.
Nit
| public readonly isActive = observableValue<boolean, void>('cellIsActive', false); | |
| public readonly isActive = observableValue('cellIsActive', false); |
Addresses #9848 #10280 #10281 #9992 which are all active cell/focus/selection related bugs and #10470 which just updates the clear output icon to match the icons we use elsewhere in the IDE.
This is a pretty large PR that addresses a number of selection behavior bugs and some selection/hover styling requests as described in the issues above. The root cause for many of these issues was that we didn't really treat a single cell as the one with focus in multi-select scenarios.
Following the commits to understand the changes would be an easy way to understand what each change is doing.
Fixed:
EscapeThere are probably other things that may have been fixed with this change but the above were intentionally fixed. I'd normally include videos but there is A LOT that has changed and capturing it all in a few videos isn't totally possible.
Not Fixed:
Hover State
Screen.Recording.2025-11-14.at.11.59.04.AM.mov
Basic Multi-Select + Active Cell Keyboard Navigation
Screen.Recording.2025-11-14.at.12.00.22.PM.mov
Release Notes
New Features
Bug Fixes
QA Notes
@:positron-notebooks