-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Promote webview Api to stable #47989
Conversation
src/vs/vscode.d.ts
Outdated
/** | ||
* Editor position of the panel. | ||
*/ | ||
readonly position?: ViewColumn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet sure about that... We can make is deprecated from the start (grid layout) so that it compares to editors. Also, since it optional is can work like the editor-case (think of embedded editors or output-editors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll make it as deprecated. The intent is that property could be extended in the future to use other locations such as panel positions:
position?: ViewColumn | PanelPosition;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option is that we change this back to viewColumn
and introduce the position
property for use with grid layout
* | ||
* Pass in an empty array to disallow access to any local resources. | ||
*/ | ||
readonly localResourceRoots?: ReadonlyArray<Uri>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Touches on #42091
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need extra doc, how do I can add an extra root? Is this alway additive to the two default locations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm writing a new section of the docs for webview. An extension can recreate the default values using extensionContext.path
and workspaceRoots
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/vs/vscode.d.ts
Outdated
/** | ||
* Title of the webview shown in UI. | ||
*/ | ||
title: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is a panel or widget property...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a panel property but I moved it to the webview. The intention is that title is a common property pretty much anywhere the webview would be used. The owner of a webview (such as the WebviewPanel
) just figures out how to display it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is that title is a common property pretty much anywhere the webview would be used.
Could we say the same of dispose? Somehow I was missing it from the panel and I would also miss that from something like WebviewWindow
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think most conceivable future api uses of the Webview
would also want a dispose
method. I'll move dispose
and onDidDispose
from the WebviewPanel
class to the Webview
itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that's the opposite of what I wanted ;-) It's kinda bogus to say createWebviewPanel
but to then call webviewPanel.webview.dispose()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you would prefer it on both objects?
/** | ||
* A webview displays html content, like an iframe. | ||
*/ | ||
export interface Webview { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we sketch-up a sample for the editor-view-zone-case? Or one in which a webview would be inside a separate window?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the branch with a webview widget (view zone) sketch: https://github.com/Microsoft/vscode/blob/mjbvz/webviewWidgetProto/src/vs/vscode.proposed.d.ts#L689
The window one would be similar. It could be as simple as a top level function called createWebviewWindow(...): Thenable<Webview>
or we could introduce a WebviewWindow
class that has window specific properties on it:
interface WebviewWindow {
readonly webview: Webview;
onWillClose: Event<void>;
onDidFocus: Event<void>;
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
One more question |
This allows us to more easily re-introduce a `position` property once we have gridlayout
Good question. Electron uses one word: I may be overthinking this. What do you think? |
Also better hide a few 'internal' methods / properties on the panel / webview
fbbbe11
to
8fab6cc
Compare
Good point, my gut wants |
Outcomes from the API sync this morning:
I've made these changes and am merging the PR. We'll continue to polish it based on any feedback from the test pass or from extension authors |
Promotes the proposed webview Api to stable
Fixes #43713
Fixes #28263