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

Add webview for when dvc is not available or not initialized #2861

Merged
merged 12 commits into from
Dec 1, 2022

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Nov 30, 2022

A start on #2776

Screen.Recording.2022-11-30.at.6.40.39.PM.mov

@sroy3 sroy3 added the product PR that affects product label Nov 30, 2022
@sroy3 sroy3 self-assigned this Nov 30, 2022
@sroy3 sroy3 marked this pull request as ready for review November 30, 2022 19:41
@sroy3
Copy link
Contributor Author

sroy3 commented Nov 30, 2022

The structure should change a lot in the following PRs. Not sure if it's worth adding tests for now. I'll add them if you think otherwise.

@sroy3 sroy3 marked this pull request as draft November 30, 2022 20:37
@@ -48,5 +66,15 @@ export abstract class BaseWorkspaceWebviews<
return overrideRoot || (await this.getFocusedOrOnlyOrPickProject())
}

protected async showEmptyWebview(viewColumn?: ViewColumn) {
await createWebview(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating a simple webview for now, but I've already started creating a class for it to add messages.

@sroy3 sroy3 marked this pull request as ready for review November 30, 2022 23:36
@@ -1351,7 +1351,7 @@
{
"view": "dvc.views.webviews",
"contents": "[Show Experiments](command:dvc.showExperiments)\n[Show Plots](command:dvc.showPlots)\n[Show Experiments and Plots](command:dvc.showExperimentsAndPlots)",
"when": "dvc.commands.available && dvc.project.available"
"when": "true"
Copy link
Member

@mattseddon mattseddon Dec 1, 2022

Choose a reason for hiding this comment

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

[Q] Would it be easier to show the same view but with different underlying commands under different circumstances? Would that simplify the code elsewhere?

i.e

      {
        "view": "dvc.views.webviews",
        "contents": "[Show Experiments](command:dvc.showExperiments)\n[Show Plots](command:dvc.showPlots)\n[Show Experiments and Plots](command:dvc.showExperimentsAndPlots)",
        "when": "dvc.commands.available && dvc.project.available"
      },
      {
        "view": "dvc.views.webviews",
        "contents": "[Show Experiments](command:dvc.showEmptyWebview)\n[Show Plots](command:dvc.showEmptyWebview)\n[Show Experiments and Plots](command:dvc.showEmptyWebview)",
        "when": "!dvc.commands.available || !dvc.project.available"
     }

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 think you're right. I'll try that. Thanks!

Copy link
Member

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

Left one [Q] that might help to simplify code paths. Just need to check that the new webview is correctly bundled with the extension.

@codeclimate
Copy link

codeclimate bot commented Dec 1, 2022

Code Climate has analyzed commit ca03d49 and detected 0 issues on this pull request.

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

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

View more on Code Climate.

@sroy3 sroy3 merged commit ef2a987 into main Dec 1, 2022
@sroy3 sroy3 deleted the get-started-views branch December 1, 2022 15:39
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