Close the browser tab when clicking on "Close and Shut Down Notebook" #6937

jtpio · 2023-06-20T06:43:43Z

Fixes #6934
Fixes #1159

This mimics the behavior of the Classic Notebook:

close-and-halt.mp4

In Notebook 7:

notebook-7-close-and-halt.mp4

Changes

Add new semantic command to the closeAndCleaners
Move the close and cleaners down in the file menu. Related to Notebook v7 menus should match the v6 ones whenever possible #6398
Keep the same "Close and Halt" name in the menu entry as in the classic UI
Update reference snapshots
Add UI test to check the browser tab is closed when clicking on the menu item

github-actions · 2023-06-20T06:43:59Z

👈 Launch a Binder on branch jtpio/notebook/close-and-shutdown

jtpio · 2023-06-20T07:18:10Z

bot please update playwright snapshots

jtpio · 2023-06-20T08:11:46Z

cc @gutow for awareness

andrii-i

This looks and works well, thank you for working on this @jtpio.

I believe there is spaceto improve user experience further beyond adding parity with nbclassic:

It's not clear what exactly "Close and Halt" and "Shut Down" items do and what is the difference between them. Please find suggestions below on renaming "Close and Halt" into "Close Notebook and Shutdown Kernel". "Shut Down" should then be renamed into "Shut Down Server". I think this can be done within this PR but can also be taken care of separately.
"Close and Halt" terminates kernel of the notebook and "Shut Down" terminates server with all kernels. When this happens all unsaved data is lost. Therefore these buttons are destructive and can lead to data loss. To remedy this, we could ask a user to confirm his action via modal/popup window. We should then also add "..." to the end of both options to indicate that selecting the item will lead to further options or require additional input. It would probably be best to do it in a separate PR after Notebook 7 launch, please let me know if you agree with the approach and I can create an issue to track it.

andrii-i · 2023-06-20T19:46:06Z

packages/notebook-extension/src/index.ts

+
+    const id = 'notebook:close-and-halt';
+    commands.addCommand(id, {
+      label: trans.__('Close and Halt'),


Suggested change

label: trans.__('Close and Halt'),

label: trans.__('Close Notebook and Shutdown Kernel'),

If going with this suggestion then we'll likely need to use Shut Down instead of Shutdown for consistency with the other menu entries.

andrii-i · 2023-06-20T19:49:15Z

ui-tests/test/notebook.spec.ts

@@ -155,4 +155,24 @@ test.describe('Notebook', () => {
    const imageName = 'notebooktools-right-panel.png';
    expect(await panel.screenshot()).toMatchSnapshot(imageName);
  });
+
+  test('Clicking on "Close and Halt" should close the browser tab', async ({


Suggested change

test('Clicking on "Close and Halt" should close the browser tab', async ({

test('Clicking on "Close Notebook and Shutdown Kernel" should close the browser tab', async ({

andrii-i · 2023-06-20T19:54:54Z

ui-tests/test/notebook.spec.ts

+    );
+    await page.goto(`notebooks/${tmpPath}/${notebook}`);
+
+    const menuPath = 'File>Close and Halt';


Suggested change

const menuPath = 'File>Close and Halt';

const menuPath = 'File>Close Notebook and Shutdown Kernel';

jtpio · 2023-06-21T06:26:20Z

It's not clear what exactly "Close and Halt" and "Shut Down" items do and what is the difference between them. Please find suggestions below on renaming "Close and Halt" into "Close Notebook and Shutdown Kernel". "Shut Down" should then be renamed into "Shut Down Server". I think this can be done within this PR but can also be taken care of separately.

Good question. JupyterLab uses "Close and Shut Down Notebook", while for the Classic Notebook it's "Close and Halt". This PR went with "Close and Halt" so it looks the same as the classic notebook and also taking into consideration #6398 (for example if folks have screenshots showing these menu entries in the course materials).

However for consistency with JupyterLab it would be fine to go with "Close and Shut Down Notebook" yes, no strong opinion.

2. To remedy this, we could ask a user to confirm his action via modal/popup window

Normally there is already a dialog (see the second screencast):

andrii-i · 2023-06-21T08:19:32Z

However for consistency with JupyterLab it would be fine to go with "Close and Shut Down Notebook" yes, no strong opinion.

From my point of view, "Close and Shut Down Notebook" is significantly more clear in explaining its function. "Close and Shut Down Notebook" still describes what happens with kernel better vs for example "Close and Halt Notebook" as "halt" implies pausing. Also we already use "Shut Down" for process termination so with "halt" we would be introducing a second term for the same action. We could also check in with other contributors during the call today.

Normally there is already a dialog (see the second screencast)

Yes, somehow missed this.

jtpio · 2023-06-21T08:23:22Z

From my point of view, "Close and Shut Down Notebook" is significantly more clear in explaining its function. "Close and Shut Down Notebook" still describes what happens with kernel better vs for example "Close and Halt Notebook" as "halt" implies pausing. Also we already use "Shut Down" for process termination so with "halt" we would be introducing a second

Yes that sounds fine too. Users familiar with the Close and Halt entry from the classic notebook would probably be not too confused if it's now name Close and Shut Down Notebook now anyway.

Co-Authored-By: Andrii Ieroshenko <[email protected]>

jtpio · 2023-06-21T12:15:07Z

bot please update playwright snapshots

jtpio · 2023-06-21T12:54:24Z

FYI @andrii-i I updated the PR to take your suggestions in consideration.

jtpio · 2023-06-22T07:41:40Z

Merging as the comments above have been addressed.

Thanks @andrii-i for the review!

jtpio added 2 commits June 20, 2023 06:28

Close the browser tab when clicking on "Close and Shut Down Notebook"

fef47d7

Move menu entry to match classic

9366957

jtpio added this to the 7.0 milestone Jun 20, 2023

github-actions bot assigned jtpio Jun 20, 2023

jtpio added the enhancement label Jun 20, 2023

jtpio added 3 commits June 20, 2023 06:54

Fix menu entries

26bc7b8

Add comment about the rank

9cab98f

Use default isEnabled

e7a8bfc

github-actions bot added 2 commits June 20, 2023 07:30

Update Playwright Snapshots

7bb1ae8

Update Playwright Snapshots

a651302

jtpio closed this Jun 20, 2023

jtpio reopened this Jun 20, 2023

Add UI test

851aaa3

jtpio marked this pull request as ready for review June 20, 2023 08:11

jtpio requested a review from andrii-i June 20, 2023 08:11

jtpio changed the title ~~Close the browser tab when clicking on "Close and Shut Down Notebook"~~ Close the browser tab when clicking on "Close and Halt" Jun 20, 2023

andrii-i approved these changes Jun 20, 2023

View reviewed changes

Merge branch 'main' into close-and-shutdown

dab98bf

Rename to "Close and Shut Down Notebook"

edea993

Co-Authored-By: Andrii Ieroshenko <[email protected]>

github-actions bot added 2 commits June 21, 2023 12:28

Update Playwright Snapshots

b3ffa66

Update Playwright Snapshots

c9f3fc6

jtpio closed this Jun 21, 2023

jtpio reopened this Jun 21, 2023

jtpio changed the title ~~Close the browser tab when clicking on "Close and Halt"~~ Close the browser tab when clicking on "Close and Shut Down Notebook" Jun 21, 2023

jtpio merged commit 77f87de into jupyter:main Jun 22, 2023

jtpio deleted the close-and-shutdown branch June 22, 2023 07:41

jtpio mentioned this pull request May 7, 2024

Menu entry for command 'application:close' is duplicated. #7358

Open

github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close the browser tab when clicking on "Close and Shut Down Notebook" #6937

Close the browser tab when clicking on "Close and Shut Down Notebook" #6937

jtpio commented Jun 20, 2023 •

edited

Loading

github-actions bot commented Jun 20, 2023

jtpio commented Jun 20, 2023

jtpio commented Jun 20, 2023

andrii-i left a comment •

edited

Loading

andrii-i Jun 20, 2023

jtpio Jun 21, 2023

andrii-i Jun 20, 2023

andrii-i Jun 20, 2023

jtpio commented Jun 21, 2023

andrii-i commented Jun 21, 2023

jtpio commented Jun 21, 2023

jtpio commented Jun 21, 2023

jtpio commented Jun 21, 2023

jtpio commented Jun 22, 2023

	label: trans.__('Close and Halt'),
	label: trans.__('Close Notebook and Shutdown Kernel'),

	test('Clicking on "Close and Halt" should close the browser tab', async ({
	test('Clicking on "Close Notebook and Shutdown Kernel" should close the browser tab', async ({

	const menuPath = 'File>Close and Halt';
	const menuPath = 'File>Close Notebook and Shutdown Kernel';

Close the browser tab when clicking on "Close and Shut Down Notebook" #6937

Close the browser tab when clicking on "Close and Shut Down Notebook" #6937

Conversation

jtpio commented Jun 20, 2023 • edited Loading

Changes

github-actions bot commented Jun 20, 2023

jtpio commented Jun 20, 2023

jtpio commented Jun 20, 2023

andrii-i left a comment • edited Loading

Choose a reason for hiding this comment

andrii-i Jun 20, 2023

Choose a reason for hiding this comment

jtpio Jun 21, 2023

Choose a reason for hiding this comment

andrii-i Jun 20, 2023

Choose a reason for hiding this comment

andrii-i Jun 20, 2023

Choose a reason for hiding this comment

jtpio commented Jun 21, 2023

andrii-i commented Jun 21, 2023

jtpio commented Jun 21, 2023

jtpio commented Jun 21, 2023

jtpio commented Jun 21, 2023

jtpio commented Jun 22, 2023

jtpio commented Jun 20, 2023 •

edited

Loading

andrii-i left a comment •

edited

Loading