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

Add token on tree widget #6496

Merged
merged 5 commits into from
Aug 5, 2022
Merged

Add token on tree widget #6496

merged 5 commits into from
Aug 5, 2022

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Aug 1, 2022

Fixes #6410

The NotebookShell allows only one widget in the main area.
As the tree view is a TabPanel, it would be interesting to be able to add widgets (as it was possible in Classic Notebook).

This PR adds a token on the NotebookTreeWidget, the widget in main area in tree view.
Any extension will now be able to optionally try to retrieve the INotebookTree in the activate function, and add widgets in it using the TabPanel's interface functions.

It should close #6478 as a more generic implementation.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Binder 👈 Launch a Binder on branch brichet/notebook/feature/tree_token

@jtpio jtpio added this to the 7.0 milestone Aug 2, 2022
packages/tree/package.json Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Aug 2, 2022

Thanks @brichet for working on this.

Diff looks good. CI is currently broken because of ypy-websocket, pending a new pre-release of JupyterLab 4.0.

@brichet
Copy link
Contributor Author

brichet commented Aug 2, 2022

I called the widget and token NotebookTree, but I wonder if it should be something like NotebookMain or NotebookRoot to avoid confusion (for me, tree refers to tree files widget but I could be wrong).

@jtpio
Copy link
Member

jtpio commented Aug 2, 2022

I called the widget and token NotebookTree

Right, this naming mostly comes from the name of the /tree endpoint.

Probably it's fine to keep that way for now while in alpha. We can always revisit later and maybe we could also rename the tree-extension as well.

Comment on lines 35 to 36
import { NotebookTreeWidget } from '@jupyter-notebook/tree';
import { INotebookTree } from '@jupyter-notebook/tree';
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these two imports could be combined?


import { INotebookTree } from './token';

export class NotebookTreeWidget extends TabPanel implements INotebookTree {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a docstring to the class and constructor for consistency with the rest of the code base?

@jtpio
Copy link
Member

jtpio commented Aug 3, 2022

The UI tests checks still appear to be cancelled after restarting them:

yarn run v1.22.17
$ playwright test --browser firefox

Running 24 tests using 1 worker
Error: The operation was canceled.

@brichet
Copy link
Contributor Author

brichet commented Aug 4, 2022

Thanks for reviewing @jtpio. I''l have a look at it.

@brichet
Copy link
Contributor Author

brichet commented Aug 4, 2022

A part of the tests are currently failing because of :

Failed to load toolbar items for factory FileBrowser from 
Error: Plugin `id` parameter is required for settings fetch.

So the tree widget toolbar is not displayed and filled with Upload and New buttons as expected.

@@ -93,10 +94,87 @@ const createNew: JupyterFrontEndPlugin<void> = {
}
};

function activateNotebookTreeWidget(
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to move the activate function to a separate function?

Maybe we can keep inlined as part of the plugin definition for consistency with the rest of the code base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I reverted it

@jtpio
Copy link
Member

jtpio commented Aug 4, 2022

Thanks @brichet this is looking good.

I wanted to check on Binder but it seems like it doesn't start v7 anymore, which also happens on main:

image

Probably something changed in the setup (maybe the hatch migration), I'll have a look in a separate PR.

@jtpio
Copy link
Member

jtpio commented Aug 4, 2022

Probably something changed in the setup (maybe the hatch migration), I'll have a look in a separate PR.

This should be fixed by #6505.

@brichet
Copy link
Contributor Author

brichet commented Aug 4, 2022

Probably something changed in the setup (maybe the hatch migration), I'll have a look in a separate PR.

This should be fixed by #6505.

Nice! But the binder link is created only when the PR opens the first time I think, so it will not work on this one.

@jtpio
Copy link
Member

jtpio commented Aug 4, 2022

The binder link is created only when the PR opens the first time I think

Right but it doesn't change, so we can open it again or refresh the page.

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit 3674afe into jupyter:main Aug 5, 2022
@brichet brichet deleted the feature/tree_token branch August 5, 2022 07:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2023
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.

Expose the tree TabPanel to it can be consumed by other extensions
2 participants