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

Lts devel #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Lts devel #2

wants to merge 3 commits into from

Conversation

rickmcgeer
Copy link
Contributor

This is the PT for the LTS (Long-Term Sustainable) branch of the jupyterlab-universal-extension. It follows the directions in @fcollonval's PR, and should be an exact copy. The only reason this is being done as a separate branch/PR from #1 is to have a branch to compare code.

@rickmcgeer rickmcgeer self-assigned this Mar 1, 2023
Comment on lines +15 to +18
export class GalyleoDocument extends DocumentWidget<
GalyleoEditor,
DocumentModel
> {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also simply define a type.

Suggested change
export class GalyleoDocument extends DocumentWidget<
GalyleoEditor,
DocumentModel
> {}
export type GalyleoDocument = DocumentWidget<GalyleoEditor, DocumentModel>;

Comment on lines +41 to +42
this.node.onmouseleave = () => (this._iframe.style.pointerEvents = 'none');
this.node.onmousemove = (evt: MouseEvent) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add listeners in onAfterAttach and remove them in onBeforeDetach.

see e.g. https://github.com/jupyterlab/jupyterlab/blob/c6180cefc57448a92a4761b66069cdd6c6f6637c/packages/cells/src/resizeHandle.ts#L16

Comment on lines +212 to +229
setOption(key: string, value: any): void {
// do nothing
}

setOptions(options: Partial<CodeEditor.IConfig>): void {
// do nothing
}

getCursorPosition(): CodeEditor.IPosition {
return {
line: 0,
column: 0
};
}

getLine(lineNumber: number): string | null {
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no constrain in JupyterLab about the API of the content widget. So I would highly suggest keeping it simple.

const extension: JupyterFrontEndPlugin<void> = {
id: PLUGIN_ID,
autoStart: true,
requires: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would strongly recommend not depending on so many tokens. Try to make as many as possible optional - especially if you wish your extension to be compatible with Notebook v7.

Good candidate are ILabShell, ICommandPalette, ILauncher, IMainMenu, ITranslator, ISettingRegistry.

I'm not seeing the goal of requesting the trackers and try to use app.shell rather than requesting ILabShell otherwise your extension won't be compatible with notebook v7.

<rect width="28" height="28" fill="black" fill-opacity="0"/>
<path d="M4 19H6V19.5H5V20.5H6V21H4V22H7V18H4V19ZM5 10H6V6H4V7H5V10ZM4 13H5.8L4 15.1V16H7V15H5.2L7 12.9V12H4V13ZM9 7V9H23V7H9ZM9 21H23V19H9V21ZM9 15H23V13H9V15Z" fill="#666666"/>
</svg>
<svg width="28" height="28" viewBox="0 0 28 28" fill="none" xmlns="http://www.w3.org/2000/svg">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I strongly advice you to use LabIcon registry and store only one version of your icons for all theme.

See https://github.com/jupyterlab/jupyterlab-git/blob/master/src/style/icons.ts

To have icon colors adapt to themes, you will need to define a class to the SVG; see

https://github.com/jupyterlab/jupyterlab-git/blob/91811aa8e127483cc807b26785a8a674397391ac/style/icons/add.svg#L7

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