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

Allow extensions to contribute custom icons for webview panels #49657

Closed
wants to merge 1 commit into from

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented May 11, 2018

Fixes #48864

Adds a new top level webview contribution point. Use this new contribution point to contribute icons to webviews declaratively

Fixes microsoft#48864

Adds a new top level `webview` contribution point. Use this new contribution point to contribute the declarative part of a webviews, such as icons
@mjbvz mjbvz self-assigned this May 11, 2018
@mjbvz mjbvz requested a review from jrieken May 11, 2018 00:46
@@ -26,6 +26,15 @@
"onView:markdown.preview"
],
"contributes": {
"webviews": [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I initially tried putting this new point under views, but now that we support custom view containers it does not seem like a great fit there

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean by 'custom view containers'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The custom activity bar sections that we added last iteration. I see two concerns with reusing views for webview contributions:

  • Webview contributions would not have the same properties as the other items in views.
  • As we now support contributing a custom view container with any name, there's nothing to stop someone from creating a view container named webviews. Pretty unlikely this will be a real problem so the first point is the main concern

Copy link
Member

Choose a reason for hiding this comment

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

If it has to be merged into the views extension point, then editor is the right location for web views instead of webviews. IMO Web view and Tree view are types of views. I see that at present Tree views and Web views do not have common schema, for e.g. Tree views do not have icons where as web views do. We can have either schema by type or schema by location depends on how they emerge in future. Looking at present scenario, it looks to me that the location drives the properties. I mean, views registered in activity bar containers do not need icons but icons are needed for views registered under editor. In future, if we want to be flexible in moving views across, then all views can define icons optionally. Not sure what is viewType property in web views. May be this can be used to differentiate tree views vs web views?

I think the second point you mentioned is a valid concern. May be we should think of restricting some default locations like editor or panel.

Or other approach would be to use viewsContainersextension point? Say, introduce a new locationeditor` and create a new view container for web views. It means a view container inside editor can contain multiple views(web or tree).

@jrieken
Copy link
Member

jrieken commented May 14, 2018

Maybe @sandy081 can add his 2 cents because he has most experience with those things

@mjbvz mjbvz requested a review from sandy081 May 14, 2018 16:00
@sandy081
Copy link
Member

If it has to be merged into the views extension point, then editor is the right location for web views instead of webviews. IMO Web view and Tree view are types of views. I see that at present Tree views and Web views do not have common schema, for e.g. Tree views do not have icons where as web views do. We can have either schema by type or schema by location depends on how they emerge in future. Looking at present scenario, it looks to me that the location drives the properties. I mean, views registered in activity bar containers do not need icons but icons are needed for views registered under editor. In future, if we want to be flexible in moving views across, then all views can define icons optionally. Not sure what is viewType property in web views. May be this can be used to differentiate tree views vs web views?

I think the second point you mentioned is a valid concern. May be we should think of restricting some default locations like editor or panel.

Or other approach would be to use viewsContainersextension point? Say, introduce a new locationeditor` and create a new view container for web views. It means a view container inside editor can contain multiple views(web or tree).

mjbvz added a commit to mjbvz/vscode that referenced this pull request Jul 23, 2018
Allows webviews to provide icons used in UI. Adds a new `WebviewPanel.iconPath` property for this.

Replaces the static contribution approach from microsoft#49657

Fixes microsoft#48864
@mjbvz
Copy link
Collaborator Author

mjbvz commented Jul 23, 2018

Superseded by #54912

@mjbvz mjbvz closed this Jul 23, 2018
mjbvz added a commit that referenced this pull request Jul 24, 2018
* Add WebviewPanel.iconPath

Allows webviews to provide icons used in UI. Adds a new `WebviewPanel.iconPath` property for this.

Replaces the static contribution approach from #49657

Fixes #48864

* Fix doc

* Move icon into mainthreadwebview

* Cleaning up implementation

* Cleaning up implementation
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow webview panels to provide a custom icon
3 participants