-
Notifications
You must be signed in to change notification settings - Fork 122
PositroNB: Restart kernel button #9898
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
Conversation
|
E2E Tests 🚀 |
|
@seeM Based on your changes, what do you expect to happen in the scenarios 2-4? Cases
Outcomes
|
|
@seeM Awesome! This is looking good however I am concerned that we are running out of space in the nav bar and not grouping related actions in the same area. I put 2 mockups for exploration here to determine next steps: https://www.figma.com/design/z9EOP4qLLzR08PNxe4M0Ei/Positron-Notebooks---Design-Audit?node-id=305-2&t=8CHFBPPjEQfYYkiB-1 |
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.
Code itself looks great. I'll leave the design part to further discussions.
src/vs/workbench/contrib/runtimeNotebookKernel/browser/runtimeNotebookKernelActions.ts
Outdated
Show resolved
Hide resolved
ca6b680 to
8b66d2d
Compare
|
Thanks for the reviews! I've updated the UI based on your feedback:
|
|
@seeM looking great! Based on the design we made the other day with a new dropdown for the kernel, are you suggesting that whole dropdown gets handled in #9722 Where restart kernel and show console would be a submenu item in this dropdown? My understanding was that 9722 would just capture putting the kernel info in the new dropdown |
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 don't see any particular e2e tests that exercise the notebook kernel restart. I can def add a test for that. However, I was curious if there are any particular checks I should assert? Is there any kind of persistence that is expected or is everything wiped?
|
@midleman in terms of state, it should behave exactly as restarting a console session. Some ideas:
|
|
@rodrigosf672 IIUC whether the notebook is saved or not shouldn't affect restart behavior. Perhaps restarting while something is running should display a warning, and perhaps a warning should also be displayed based if data will be lost. Will start a discussion in Slack. |
|
FYI, added a ticket to create an e2e test for this feature: #10033 |
202ebc4 to
fbd23f2
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.
Re-reviewed and all looks good to me! I like the new design!
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 all this work!
I marked as Request changes because of the runtime status icon bug which does effect the console session list too. The fix is a small one line change which I dropped as a code suggestion. Mostly looking great. Had a couple comments/questions but nothing majorly blocking once that status icon color bug is fixed!
src/vs/workbench/contrib/positronConsole/browser/components/runtimeStatus.tsx
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronConsole/browser/components/consoleInstanceState.tsx
Show resolved
Hide resolved
src/vs/workbench/contrib/positronConsole/browser/components/runtimeStatus.tsx
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronNotebook/browser/KernelStatusBadge.tsx
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronNotebook/browser/KernelStatusBadge.css
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronNotebook/browser/positronNotebook.contribution.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronNotebook/browser/SelectPositronNotebookKernelAction.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Dhruvi Sompura <[email protected]> Signed-off-by: Wasim Lorgat <[email protected]>
e1fe2f5 to
8ab40eb
Compare


Add the Restart Kernel button to Positron notebooks, addresses #9789. The button lives in the new Kernel Actions menu, addressing #9722. The Kernel Actions button shows the runtime status and name, matching the style used in the console.
Also removed the titles from Run All and Clear Outputs buttons, and moved + Code and + Markdown to the left.
Release Notes
New Features
Bug Fixes
QA Notes
@:notebooks