Skip to content

Add abstract interface manager#1478

Merged
koesie10 merged 3 commits intomainfrom
koesie10/abstract-interface-manager
Aug 31, 2022
Merged

Add abstract interface manager#1478
koesie10 merged 3 commits intomainfrom
koesie10/abstract-interface-manager

Conversation

@koesie10
Copy link
Copy Markdown
Member

This will add a new abstract class that implements the creation of the panel and webview to reduce duplication across the different interface managers.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

This will add a new abstract class that implements the creation of the
panel and webview to reduce duplication across the different interface
managers.
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Quick review. Haven't tried this out yet. Looks good so far.

...(config.additionalOptions?.localResourceRoots ?? []),
Uri.file(tmpDir.name),
Uri.file(path.join(ctx.extensionPath, 'out')),
Uri.file(path.join(ctx.extensionPath, 'node_modules/@vscode/codicons/dist')),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In your other PR, you have codicons being loaded for the MRVA view only. Here it looks like you are loading for all the views. I think what you have here is fine (since I think we should use codicons in the other views where appropriate), but I just want to make sure I understand this properly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You might have gotten this from the previous PR already, but it's indeed correct that we are now loading it for all the views.

@koesie10 koesie10 mentioned this pull request Aug 29, 2022
3 tasks
Base automatically changed from koesie10/unified-webpack-bundle to main August 30, 2022 09:29
@koesie10 koesie10 marked this pull request as ready for review August 30, 2022 09:29
@koesie10 koesie10 requested review from a team as code owners August 30, 2022 09:29
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Tested this locally. Seems to be working. LGTM.

@koesie10 koesie10 merged commit cf36a52 into main Aug 31, 2022
@koesie10 koesie10 deleted the koesie10/abstract-interface-manager branch August 31, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants