Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add token on tree widget#6496

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jtpio merged 5 commits intojupyter:mainfrombrichet:feature/tree_token
Aug 5, 2022
Merged

Conversation

@brichet
Copy link
Collaborator

@brichetbrichet commentedAug 1, 2022•
edited by jtpio
Loading

Fixes#6410

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

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

It shouldclose#6478 as a more generic implementation.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branchbrichet/notebook/feature/tree_token

@jtpiojtpio added this to the7.0 milestoneAug 2, 2022
@jtpio
Copy link
Member

Thanks@brichet for working on this.

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

@brichet
Copy link
CollaboratorAuthor

I called the widget and tokenNotebookTree, but I wonder if it should be something likeNotebookMain orNotebookRoot to avoid confusion (for me,tree refers to tree files widget but I could be wrong).

@jtpio
Copy link
Member

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 thetree-extension as well.

brichet reacted with thumbs up emoji

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';

exportclassNotebookTreeWidgetextendsTabPanelimplementsINotebookTree{
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 andconstructor for consistency with the rest of the code base?

@jtpio
Copy link
Member

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

yarn run v1.22.17$ playwright test --browser firefoxRunning 24 tests using 1 workerError: The operation was canceled.

@brichet
Copy link
CollaboratorAuthor

brichet commentedAug 4, 2022•
edited
Loading

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

@brichet
Copy link
CollaboratorAuthor

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 withUpload andNew buttons as expected.

}
};

functionactivateNotebookTreeWidget(
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
CollaboratorAuthor

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

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 onmain:

image

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

brichet reacted with thumbs up emoji

@jtpio
Copy link
Member

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
CollaboratorAuthor

brichet commentedAug 4, 2022•
edited
Loading

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

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.

brichet reacted with laugh emoji

Copy link
Member

@jtpiojtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpiojtpio merged commit3674afe intojupyter:mainAug 5, 2022
@brichetbrichet deleted the feature/tree_token branchAugust 5, 2022 07:02
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsAug 6, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@jtpiojtpiojtpio approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

7.0

Development

Successfully merging this pull request may close these issues.

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

2 participants

@brichet@jtpio
[8]ページ先頭
©2009-2025 Movatter.jp