Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add logout button #6087

Merged
merged 2 commits into from
May 2, 2019
Merged

Add logout button #6087

merged 2 commits into from
May 2, 2019

Conversation

Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Mar 10, 2019

Fixes #5966

Is there an easy way to change the order a phosphor widget is inserted ? I've not been able to find a trivial way to add it to the end of the shell's top area - so it shows up at top left of the screen.

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@Madhu94
Copy link
Contributor Author

Madhu94 commented Mar 10, 2019 •
edited
Loading

The limitation seems to be with this line here. Are there any plans to make app.shell's positioning API more granular ? Like, app.shell.addWidget(widget, "top right" ?

@jtpio jtpio mentioned this pull request Mar 11, 2019
Closed
@afshin afshin added this to the 1.0 milestone Mar 27, 2019
@afshin afshin requested review from afshin and tgeorgeux March 27, 2019 16:37
@ian-r-rose
Copy link
Member

Thanks @Madhu94. We had some discussion of button placement style and location in #4414, and decided that it was better to just have it in the "File" menu with other application-level commands. I think the best solution would be to do the same here, so we sidestep the design issue of where to put the button. So I suggest doing the following:

  1. Change the button to a command, and add it to the File menu.
  2. Give the "shutdown" and "logout" commands descriptive names so that the user is not confused. I think "Quit" is kind of misleading, so perhaps we should have them be named "Logout" and "Shutdown" instead.

What do you think?

@Madhu94
Copy link
Contributor Author

Madhu94 commented Apr 2, 2019 •
edited
Loading

Thanks for getting back @ian-r-rose. A command seems like a great idea, will change this implementation. The Logout/Shutdown distinction sounds good too.

@meeseeksmachine
Copy link
Contributor

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/problems-with-installing-jupyterhub-on-windows-and-using-jupyterlab/682/5

@jasongrout
Copy link
Contributor

@Madhu94 - just checking in on this. How are things going? I was hoping to review it and get it in soon.

@jasongrout jasongrout changed the title Add logout button [WIP] Add logout button Apr 22, 2019
@jasongrout
Copy link
Contributor

Added wip to label to help categorize.

@Madhu94
Copy link
Contributor Author

Madhu94 commented Apr 22, 2019

I was offline for a few weeks, apologies for the delay! I changed it to a command, please review

jasongrout
jasongrout reviewed Apr 22, 2019
View reviewed changes
packages/mainmenu-extension/src/index.ts Outdated
execute: () => {
return showDialog({
title: 'Quit confirmation',
title: 'Shutdown confirmation',
body: 'Please confirm you want to quit JupyterLab.',
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also change this text and warnButton below from "quit"

jasongrout
jasongrout reviewed Apr 22, 2019
View reviewed changes
packages/mainmenu-extension/src/index.ts Outdated
@@ -383,6 +388,14 @@ export function createFileMenu(
}
});

commands.addCommand(CommandIDs.logout, {
label: 'Logout',
caption: 'Logout of JupyterLab',
Copy link
Contributor

Choose a reason for hiding this comment

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

"Log out of JupyterLab"

jasongrout
jasongrout reviewed Apr 22, 2019
View reviewed changes
packages/mainmenu-extension/src/index.ts Outdated
caption: 'Quit JupyterLab',
commands.addCommand(CommandIDs.shutdown, {
label: 'Shutdown',
caption: 'Shutdown JupyterLab',
Copy link
Contributor

@jasongrout jasongrout Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

"Shut down JupyterLab" (three words instead of two)? Does that read better?

ian-r-rose
ian-r-rose approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks @Madhu94, and thanks for your patience!

This was referenced Apr 26, 2019
Closed
Open
@Madhu94
Copy link
Contributor Author

Madhu94 commented Apr 30, 2019

I'll ping sooner, after I make the changes next time. :) Please take a look at the reworded captions

jasongrout
jasongrout reviewed Apr 30, 2019
View reviewed changes
packages/mainmenu-extension/src/index.ts
@@ -383,6 +391,14 @@ export function createFileMenu(
}
});

commands.addCommand(CommandIDs.logout, {
label: 'Logout',
caption: 'Log out of JupyterLab',
Copy link
Contributor

@jasongrout jasongrout Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

title: 'Logout confirmation',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a confirmation dialog for logout, should I add one? The classic notebook didn't

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's the difference, we don't need a title because we don't have a dialog? Okay, never mind. Let's go with the classic behavior for now, unless there is a strong reason to change.

@jasongrout
Copy link
Contributor

I'll ping sooner, after I make the changes next time. :) Please take a look at the reworded captions

Thanks. We don't get notifications when changes are pushed, only when comments are added.

@jasongrout
Copy link
Contributor

Looks good, and looks like the windows js tests failure is unrelated. Thanks again!

@jasongrout jasongrout merged commit fecd1f2 into jupyterlab:master May 2, 2019
@jasongrout jasongrout mentioned this pull request May 2, 2019
Merged
@fcollonval fcollonval mentioned this pull request May 2, 2019
Open
@Madhu94
Copy link
Contributor Author

Madhu94 commented May 2, 2019 via email

@ian-r-rose
Copy link
Member

Thanks for your work @Madhu94!

@Madhu94 Madhu94 changed the title [WIP] Add logout button Add logout button May 23, 2019
@ian-r-rose ian-r-rose mentioned this pull request Jun 6, 2019
Merged
@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
@jasongrout jasongrout added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ian-r-rose ian-r-rose ian-r-rose approved these changes

@jasongrout jasongrout jasongrout requested changes

@afshin afshin Awaiting requested review from afshin

@tgeorgeux tgeorgeux Awaiting requested review from tgeorgeux

Assignees
No one assigned
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Backwards compatible tag:Design and UX
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

No logout button
5 participants
@Madhu94 @ian-r-rose @meeseeksmachine @jasongrout @afshin