-
Notifications
You must be signed in to change notification settings - Fork 673
Extract leetcode webview base class & add md settings listener #270
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
Changes from 6 commits
d4c026f
4c34172
d0ccd53
82ec804
df68b0e
90c9480
9f0999e
85a7164
084e844
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
// Copyright (c) jdneo. All rights reserved. | ||
// Licensed under the MIT license. | ||
|
||
import { ConfigurationChangeEvent, Disposable, ExtensionContext, ViewColumn, WebviewPanel, window, workspace } from "vscode"; | ||
import { markdownEngine } from "./markdownEngine"; | ||
|
||
export abstract class LeetCodeWebview implements Disposable { | ||
|
||
protected panel: WebviewPanel | undefined; | ||
private context: ExtensionContext; | ||
private listener: Disposable; | ||
|
||
public initialize(context: ExtensionContext): void { | ||
const { onDidChangeConfiguration } = this.getWebviewOption(); | ||
this.context = context; | ||
if (onDidChangeConfiguration) { | ||
this.listener = workspace.onDidChangeConfiguration(onDidChangeConfiguration, this); | ||
} else { | ||
this.listener = workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => { | ||
if (event.affectsConfiguration("markdown") && this.panel) { | ||
this.panel.webview.html = this.getWebviewContent(); | ||
} | ||
}, this); | ||
} | ||
} | ||
|
||
public dispose(): void { | ||
this.listener.dispose(); | ||
if (this.panel) { | ||
this.panel.dispose(); | ||
} | ||
} | ||
|
||
protected showWebviewInternal(): this is { panel: WebviewPanel } { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain more about this method? The return type is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It reduced the scope of union type: function isTypeDefined(obj: Type | undefined): obj is Type {
return obj !== undefined;
}
let a: Type | undefined;
a.method() // ts error, a may be undefined
if (isTypeDefined(a)) {
a.method(); // now a's type is only `Type`, `undefined` is reduced
}
So the code here do not need to use if (this.showWebviewInternal()) {
this.panel.title = `${node.name}: Preview`; // now this.panel's undefined type is reduced
this.panel.reveal(ViewColumn.One);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then why we are not just calling I do not see any benefits to do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean the code below? this.showWebviewInternal();
this.panel!.title = `${node.name}: Preview`; // this.panel's undefined type is not reduced
this.panel!.reveal(ViewColumn.One); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main problem here is, we should extract the fields in
So after the extraction, what you need to do is:
this.title = xxxx
this.showWebviewInternal(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've brought everything except There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really think there is no relationship? Did you ever test yourself what is the visual effect when the title keeps Why do you pay more attention to something in the next PR which I haven't given illustration yet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of cause I need to understand what you will do in the next PR since they have some relationship. This will determine whether we need to keep The codes written in a good way should not only have good experience but also good at self-explain. This is very important to attract more community contribution. That's why I'm keeping asking you those questions. From the code itself, I cannot get your point. There's no GIF/picture to illustrate that. Then how could I know your point is that the tab is so long that make the experience bad? Besides, From my understanding, the reason to rename to the title to Take the VS Code embedded Markdown extension as an example, You can see there is no optimization when the tab is too long. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not saying that we should not do the optimation. It's acceptable to do it as long as the code logic is easy to understand. |
||
if (!this.panel) { | ||
const { viewType, title, viewColumn, onDidReceiveMessage } = this.getWebviewOption(); | ||
|
||
this.panel = window.createWebviewPanel(viewType, title, viewColumn || ViewColumn.One, { | ||
enableScripts: true, | ||
enableCommandUris: true, | ||
enableFindWidget: true, | ||
retainContextWhenHidden: true, | ||
localResourceRoots: markdownEngine.localResourceRoots, | ||
}); | ||
|
||
this.panel.onDidDispose(() => { | ||
this.panel = undefined; | ||
}, null, this.context.subscriptions); | ||
|
||
if (onDidReceiveMessage) { | ||
this.panel.webview.onDidReceiveMessage(onDidReceiveMessage, this, this.context.subscriptions); | ||
} | ||
} | ||
|
||
this.panel.webview.html = this.getWebviewContent(); | ||
return true; | ||
} | ||
|
||
protected abstract getWebviewOption(): ILeetCodeWebviewOption; | ||
|
||
protected abstract getWebviewContent(): string; | ||
} | ||
|
||
export interface ILeetCodeWebviewOption { | ||
viewType: string; | ||
title: string; | ||
viewColumn?: ViewColumn; | ||
onDidReceiveMessage?: (message: any) => Promise<void>; | ||
onDidChangeConfiguration?: (event: ConfigurationChangeEvent) => Promise<void>; | ||
} |
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.