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

Context menu hint #5940

Merged
merged 3 commits into from Feb 14, 2019
Merged

Context menu hint #5940

merged 3 commits into from Feb 14, 2019

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 4, 2019

Fixes #4023.

This adds a new context menu item to body which instructs the user how to open the native browser menu.

There is one outstanding issue to figure out: currently, if the application context menu has no items in it, the native one is shown. Since this adds an item to body that is always visible, that makes the native context menu always invisible without Shift, at least by default.

I have added some additional logic to catch this case. If only the "context menu info" item is showing, this instead closes the application context menu and allows the native one to open. I am not sure whether this is a good idea: it may be better to just always show the application one. In that case, we only want the first commit of this PR.

cc @tgeorgeux @afshin

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 6, 2019

In the dev meeting today most people were leaning towards using the behavior that this currently has: if there are no actions available, dispose of the context menu and show the native one.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Feb 6, 2019

I am ringing to add a bit to think about as well as in agreement with @ian-r-rose 's comment above. I think there are two main reasons users right click on something, one of those is to expose more commands within JupyterLab, and the other is to expose the native context menu.

Within workflow in JupyterLab I think that more often you're looking to expose the JL context menu, not the native browser menu. In development, I don't think this is a safe assumption, but I also think we should design more with end users than developers in mind (Even though sometimes those are the same person). I think it's as easy assumption to make that end users are more often trying to expose the JL context menu.

With that in mind, what are the stakes if somebody is trying to expose the native context menu, and they end up with an empty JL context menu? They get to learn how to expose the native context, which will also help them the next time they're trying to expose a native context menu.

TL:DR Leave it as is, we can always iterate later.

@afshin
Copy link
Member

@afshin afshin commented Feb 7, 2019

With that in mind, what are the stakes if somebody is trying to expose the native context menu, and they end up with an empty JL context menu? They get to learn how to expose the native context, which will also help them the next time they're trying to expose a native context menu.

This sounds like you are saying that even when there is no JupyterLab context menu, the empty menu with the hint should show up. (a)

TL:DR Leave it as is, we can always iterate later.

But this is the opposite of that. (b)

I think we agreed on (b), keep the behavior where:

  • If the JupyterLab context menu has commands to show, show that along with the hint.
  • If the JupyterLab context menu has nothing to show except the hint, hide it and allow the native context menu to come through.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Feb 7, 2019

Yep, @afshin is right, this already has the desired behavior, where it allows the native context menu to go through if only the hint text would be shown.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Feb 7, 2019

@afshin your analysis is correct, I didn't understand the current state of the PR when we discussed in person. I thought the current behavior is the opposite of what it is.

it may be better to just always show the application one. In that case, we only want the first commit of this PR.

I'm of the opinion this is the correct move, from the end user point of view, not necessarily the developer point of view. But I don't have any data to back it up, it's just my thought when I think through the use cases from the point of view of the user. To me, the native browser menu is a bit distracting from most tasks in JupyterLab, and the user doesn't have to try an distinguish between whether this is a native or a JupyterLab menu.

I don't mind merging as is. Maybe we should make a tag for closed issues/PRs that's something along the lines of "Needs testing" so we can go into testing events with more specific goals in mind.

@ian-r-rose ian-r-rose force-pushed the context-menu-hint branch from 1c00533 to d0e9316 Feb 12, 2019
afshin
afshin approved these changes Feb 14, 2019
Copy link
Member

@afshin afshin left a comment

Let's merge this and we can modify the behavior if we want in a later PR. Thanks!

@afshin afshin merged commit f62523b into jupyterlab:master Feb 14, 2019
9 checks passed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 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.

3 participants