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

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 commented Jun 21, 2019

image

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

+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

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

Choose a reason for hiding this comment

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

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

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.

@ian-r-rose
Copy link
Member

Looks like a legitimate test failure.

@saulshanabrook saulshanabrook self-requested a review June 21, 2019 22:17
@ivanov
Copy link
Member Author

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

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?

@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.
Labels
pkg:help pkg:inspector status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants