NotificationsYou must be signed in to change notification settings
Fork5.5k
Star12.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

jtpio merged 12 commits intojupyter:mainfromjtpio:close-and-shutdown

Jun 22, 2023

Merged

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

jtpio merged 12 commits intojupyter:mainfromjtpio:close-and-shutdown

Jun 22, 2023

Conversation

Copy link

Member

jtpio commentedJun 20, 2023•
edited
Loading

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 thecloseAndCleaners
Move the close and cleaners down in the file menu. Related toNotebook 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

jtpio added2 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 the7.0 milestone

Jun 20, 2023

github-actionsbot assignedjtpio

Jun 20, 2023

Copy link

Contributor

github-actionsbot commentedJun 20, 2023

👈 Launch a Binder on branchjtpio/notebook/close-and-shutdown

jtpio added the enhancement label

Jun 20, 2023

jtpio added3 commits

June 20, 2023 06:54

Fix menu entries

26bc7b8

Add comment about the rank

9cab98f

Use defaultisEnabled

e7a8bfc

Copy link

MemberAuthor

jtpio commentedJun 20, 2023

bot please update playwright snapshots

github-actionsbot added2 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

Copy link

MemberAuthor

jtpio commentedJun 20, 2023

cc@gutow for awareness

jtpio requested a review fromandrii-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

Copy link

Contributor

andrii-i left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

packages/notebook-extension/src/index.ts Outdated


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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	label:trans.__('Close andHalt'),
	label:trans.__('CloseNotebookandShutdown Kernel'),

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If going with this suggestion then we'll likely need to useShut Down instead ofShutdown for consistency with the other menu entries.

ui-tests/test/notebook.spec.ts Outdated

		expect(awaitpanel.screenshot()).toMatchSnapshot(imageName);
		});

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	test('Clicking on "Close andHalt" should close the browser tab',async({
	test('Clicking on "CloseNotebookandShutdown Kernel" should close the browser tab',async({

ui-tests/test/notebook.spec.ts

		);
		awaitpage.goto(`notebooks/${tmpPath}/${notebook}`);

		constmenuPath='File>Close and Halt';

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	constmenuPath='File>Close andHalt';
	constmenuPath='File>CloseNotebookandShutdown Kernel';

Copy link

MemberAuthor

jtpio commentedJun 21, 2023

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):

Merge branch 'main' into close-and-shutdown

dab98bf

Copy link

Contributor

andrii-i commentedJun 21, 2023

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.

Copy link

MemberAuthor

jtpio commentedJun 21, 2023

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 theClose and Halt entry from the classic notebook would probably be not too confused if it's now nameClose and Shut Down Notebook now anyway.

Rename to "Close and Shut Down Notebook"

edea993

Co-Authored-By: Andrii Ieroshenko <ieroshenkoa@gmail.com>

Copy link

MemberAuthor

jtpio commentedJun 21, 2023

bot please update playwright snapshots

github-actionsbot added2 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

Copy link

MemberAuthor

jtpio commentedJun 21, 2023

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

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

Copy link

MemberAuthor

jtpio commentedJun 22, 2023

Merging as the comments above have been addressed.

Thanks@andrii-i for the review!

jtpio merged commit77f87de intojupyter: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-actionsbot locked asresolvedand limited conversation to collaborators

Jun 22, 2024

Labels

enhancement

Movatterモバイル変換

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

Uh oh!

Conversation

jtpio commentedJun 20, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Changes

Uh oh!

github-actionsbot commentedJun 20, 2023

Uh oh!

jtpio commentedJun 20, 2023

Uh oh!

jtpio commentedJun 20, 2023

Uh oh!

andrii-i left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrii-iJun 20, 2023

Choose a reason for hiding this comment

Uh oh!

jtpioJun 21, 2023

Choose a reason for hiding this comment

Uh oh!

andrii-iJun 20, 2023

Choose a reason for hiding this comment

Uh oh!

andrii-iJun 20, 2023

Choose a reason for hiding this comment

Uh oh!

jtpio commentedJun 21, 2023

Uh oh!

andrii-i commentedJun 21, 2023

Uh oh!

jtpio commentedJun 21, 2023

Uh oh!

jtpio commentedJun 21, 2023

Uh oh!

jtpio commentedJun 21, 2023

Uh oh!

jtpio commentedJun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jtpio commentedJun 20, 2023•
edited
Loading

andrii-i left a comment•
edited
Loading