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

Spike: Webview Row Context Menu #1519

Closed
wants to merge 6 commits into from

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Apr 2, 2022

#1518 <- this

This PR adds a React/Tippy-based context menu to the experiments table, invoked by right-clicking any experiment or checkpoint row.

context-menu-demo.mp4

Current problems:

  • Other types of tooltips can appear and hover over the context menu
  • Dismissing the menu by clicking on a row toggles the row
  • No "queue copy" or "vary params and queue" command (no simple command to wire to yet)
  • we obviously don't want option to just be the default link styles
  • Menu still appears and allows user to run commands when experiments are running

@rogermparent rogermparent changed the base branch from main to row-component-modules April 2, 2022 05:18
@codeclimate
Copy link

codeclimate bot commented Apr 2, 2022

Code Climate has analyzed commit 50e1f73 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 96.4% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.4% (-0.1% change).

View more on Code Climate.

@rogermparent rogermparent self-assigned this Apr 2, 2022
@rogermparent rogermparent added the product PR that affects product label Apr 2, 2022
@rogermparent rogermparent changed the title Row context menu Spike: Webview Row Context Menu Apr 4, 2022
@mattseddon
Copy link
Member

Note: IMO we need to match the style of the default context menus available in VS Code (default chromium context menus???):

image

@rogermparent
Copy link
Contributor Author

Note: IMO we need to match the style of the default context menus available in VS Code (default chromium context menus???):

That'll be a bit difficult considering they're just native context menus and very different between OSes, like between my system and yours for example:

image

You could say that they at least share light-on-dark, but I'd bet Windows and some Linux configurations would lead to dark-on-light context menus. There's no way we'll get an exact match without actually using native context menus (which seems not to be possible currently), so I think failing that we should focus on consistency with the rest of the webview instead.

@mattseddon
Copy link
Member

Note: IMO we need to match the style of the default context menus available in VS Code (default chromium context menus???):

That'll be a bit difficult considering they're just native context menus and very different between OSes, like between my system and yours for example:

image

You could say that they at least share light-on-dark, but I'd bet Windows and some Linux configurations would lead to dark-on-light context menus. There's no way we'll get an exact match without actually using native context menus (which seems not to be possible currently), so I think failing that we should focus on consistency with the rest of the webview instead.

It is almost possible to do something useful with existing commands (editor.action.showContextMenu):

Screen.Recording.2022-04-05.at.12.50.43.pm.mov

But looks like we will have to wait: microsoft/vscode#54285. Hopefully, this will be available soon.

(providedExperiment?: string) =>
experiments.getQueuedExpThenRun(
AvailableCommands.EXPERIMENT_REMOVE,
providedExperiment
Copy link
Member

Choose a reason for hiding this comment

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

I actually think it would be easier/less obfuscated to pass the normal message back to the extension and use the internals/setup specific analytics for these context menu actions.

@mattseddon
Copy link
Member

Messing around just gave me an idea:

Screen.Recording.2022-04-05.at.12.57.36.pm.mov

Instead of using a context menu we could have right-click trigger a quickPick with the id preloaded. It would tell the user which experiment the command would be run against and shows the available commands. Could be a nice/quick interim solution whilst we wait for access to the native context menu.

@rogermparent
Copy link
Contributor Author

Instead of using a context menu we could have right-click trigger a quickPick with the id preloaded. It would tell the user which experiment the command would be run against and shows the available commands. Could be a nice/quick interim solution whilst we wait for access to the native context menu.

Even if a little less traditional or seamless, I'm all for it! Let's bring it up at the meeting.

@mattseddon
Copy link
Member

I'll investigate this today:

image

@mattseddon
Copy link
Member

45 minutes I'll never get back:

Screen.Recording.2022-04-06.at.10.34.10.am.mov

🤸🏻

@rogermparent
Copy link
Contributor Author

Closing (at least for now) in favor of a more native-looking menu provided via quickpick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants