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.

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

move contextual help to the help menu #6678

Merged
merged 6 commits into from Jun 21, 2019

Conversation

@ivanov
Copy link
Member

@ivanov ivanov commented Jun 21, 2019

No description provided.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jun 21, 2019

Thanks for making a pull request to JupyterLab!

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

@ivanov
Copy link
Member Author

@ivanov ivanov commented Jun 21, 2019

image

@ivanov ivanov added this to the 1.0 milestone Jun 21, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

+1 to moving it to the help menu.

Can we make it a separate group just after the About and Launch classic? I think it really is different than those items. So the menu would be:

About
Launch classic
----
Open Contextual Help
-----
JupyterLab Reference
JupyterLab FAQ
Jupyter Reference
Markdown Reference
---
<rest of items>

(Notice I also rearranged the jlab/jupyter reference group - I think this order makes more sense, to put the jlab stuff first)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

Hmmm---and thinking about it, most of those items are "open" actions. It's a bit weird that only one of them says "Open". @ellisonbg, thoughts?

@ivanov
Copy link
Member Author

@ivanov ivanov commented Jun 21, 2019

Discussed this in person with @ellisonbg, who was 50-50 on remove or keeping "Open " in the menu name. At first I was going to keep it, but then I figured that in the command palette, type "open" would select the contextual help as the first item, which is not a good user experience, so I dropped open, here's what it looks like now (and as a bonus, contextual help is fairly high in the command listing with an empty entry, so it's somewhat discoverable that way, too. We will definitely want to let users know about this rename and move of the menu item in the changelog.

image

Copy link
Member

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

Thanks @ivanov, like it needs a rebase.

packages/inspector-extension/src/index.ts Show resolved Hide resolved
@@ -157,6 +160,13 @@ function activate(
command => ({ command })
);
helpMenu.addGroup(labGroup, 0);

// Contextual help in its own group
const contextualHelpGroup = [inspector ? 'inspector:open' : null].map(
Copy link
Member

@ian-r-rose ian-r-rose Jun 21, 2019

Choose a reason for hiding this comment

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

I like this check. We should do more of these.

@ivanov ivanov force-pushed the move-contextual-help branch from 067f597 to 5936a19 Jun 21, 2019
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 21, 2019

Looks like a legitimate test failure.

@saulshanabrook saulshanabrook self-requested a review Jun 21, 2019
@ivanov
Copy link
Member Author

@ivanov ivanov commented Jun 21, 2019

@saulshanabrook was concerned about the [null].map(...), but I checked and the menu item works and just gets filtered out.:
image

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 21, 2019

This works even if the inspector is not available. If you want to change the [null] behavior, can you do it in a follow-up, @saulshanabrook?

@ian-r-rose ian-r-rose merged commit f4829a7 into jupyterlab:master Jun 21, 2019
9 checks passed
@lock
Copy link

@lock 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants