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

Split the Document Manager extension into multiple plugins #12701

Merged
merged 20 commits into from Oct 11, 2022

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Jun 15, 2022

References

Follow-up to #12349

Fixes #12965

This will help with jupyter/notebook#6446 to avoid duplicating too much logic in the Notebook v7 code base.

Code changes

Split some of the docmanager extension logic into multiple plugins so they are easier to reuse and compose downstream.

New plugins:

  • @jupyterlab/docmanager-extension:opener: a plugin to open document widgets
  • @jupyterlab/docmanager-extension:contexts: handle dirty contexts

TODO

  • Split plugins
  • Add docstrings
  • Fix tests
  • Fix examples
  • Add to extension migration guide

User-facing changes

This fixes an issue introduced in #12349 not being able to create a new view for a document widget:

image

This was because the name opener was used in the code, but was not referring to the widget opener but implicitly to window.opener instead: https://developer.mozilla.org/en-US/docs/Web/API/Window/opener

Backwards-incompatible changes

None at the API level

  • The IDocumentWidgetOpener interface also now defines an opened signal that is emitted when a widget is opened.
  • Some plugins have their dependencies changed.

@jtpio jtpio added this to the 4.0.0 milestone Jun 15, 2022
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@jtpio jtpio force-pushed the docmanager-plugins branch 4 times, most recently from 498b097 to 14dc983 Compare October 10, 2022 07:11
@jtpio jtpio added the bug label Oct 10, 2022
@jtpio jtpio marked this pull request as ready for review October 10, 2022 18:44
@jtpio jtpio marked this pull request as draft October 11, 2022 07:27
@jtpio jtpio added the api-change A change that should be accompanied by a major version increase label Oct 11, 2022
@jtpio jtpio marked this pull request as ready for review October 11, 2022 07:33
@jtpio
Copy link
Member Author

jtpio commented Oct 11, 2022

cc @fcollonval for review, since you also had a look at this in #12966

Copy link
Member

@fcollonval fcollonval 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

I have a minor suggestion. Otherwise this is good to go.

packages/docmanager-extension/src/index.tsx Outdated Show resolved Hide resolved
@fcollonval
Copy link
Member

fcollonval commented Oct 11, 2022

@jtpio sorry I missed the instantiation of this._signal. So instead of Object.freeze an anonymous class would be better.

@jtpio
Copy link
Member Author

jtpio commented Oct 11, 2022

ah right, at least CI caught it.

I'll push a fix.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks

@fcollonval fcollonval merged commit 30db076 into jupyterlab:master Oct 11, 2022
@jtpio jtpio deleted the docmanager-plugins branch October 11, 2022 13:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

open a new view is broken
2 participants