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 option to open a notebook in NbClassic if it is enabled; show "Open in..." dropdown menu if there are multiple options, show single button otherwise #6866

Merged
merged 32 commits into from
Jun 9, 2023

Conversation

andrii-i
Copy link
Contributor

@andrii-i andrii-i commented May 11, 2023

  • Dynamically detect if NbClassic is enabled
  • If NbClassic is installed, add option to open a notebook in NbClassic to "Open in..." menu, command palette, "View" menu
  • Show "Open in..." dropdown menu if there are multiple options, show single button otherwise
  • Fix some lint errors in test files

Fixes #6746
Fixes #6792

"Open in..." dropdown menu
Screenshot 2023-05-11 at 1 14 26 PM
Open_in

Single button
Screenshot 2023-05-12 at 10 06 55 AM

Command palette
Screenshot 2023-05-11 at 1 16 59 PM

"View" menu
Screenshot 2023-05-11 at 1 17 58 PM

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch andrii-i/notebook/detect_nbclassic_dynamically

@jtpio
Copy link
Member

jtpio commented May 11, 2023

Thanks @andrii-i for working on this.

  • changes introduced in

Since this feature is specific to the notebook / nbclassic use case, maybe we could add the logic in the notebook repo directly instead of jupyter-server/jupyter_server#1272?

Probably the page config option could be set somewhere here:

def get_page_config(self):

@andrii-i
Copy link
Contributor Author

@jtpio thank you for looking into this PR. I was under assumption that because nbclassic is a server extension I need to expose this information in https://github.com/jupyter-server/jupyter_server through API endpoint or PageFile. Is it possible to detect if nbclassic is installed from lab-extension alone? How? I'd be happy to try.

@JasonWeill
Copy link
Collaborator

The PyPI docs capitalize the project as NbClassic (first C capitalized): https://pypi.org/project/nbclassic/

@andrii-i andrii-i changed the title Add option to open notebook in nbclassic if it is installed Add option to open a notebook in NbClassic if it is installed May 11, 2023
@andrii-i
Copy link
Contributor Author

Thank you @JasonWeill. Updated capitalization in user-facing commandLabel and commandDescription to NbClassic, also updated screenshots in the PR description.

Copy link
Collaborator

@JasonWeill JasonWeill left a comment

Choose a reason for hiding this comment

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

Consistent interCaps

packages/lab-extension/src/index.ts Outdated Show resolved Hide resolved
packages/lab-extension/src/index.ts Outdated Show resolved Hide resolved
packages/lab-extension/src/index.ts Outdated Show resolved Hide resolved
packages/lab-extension/src/index.ts Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented May 12, 2023

thank you for looking into this PR. I was under assumption that because nbclassic is a server extension I need to expose this information in https://github.com/jupyter-server/jupyter_server through API endpoint or PageFile. Is it possible to detect if nbclassic is installed from lab-extension alone? How? I'd be happy to try.

I would assume it should be possible to detect it here from app.serverapp similar to what is done here?

https://github.com/jupyter/notebook/blob/573becfa6470ce2b9b1a1451c5a54955d57b9c7a/notebook/app.py#LL52C28-L52C41

Also it would be fine to have that logic here in the notebook Python code, since it's just about passing an additional page config option. Which means it's not specific to the lab extension only, but in this case it should be fine.

@andrii-i andrii-i changed the title Add option to open a notebook in NbClassic if it is installed Add option to open a notebook in NbClassic if it is installed; show "Open in..." dropdown menu if there are multiple options, show single button otherwise May 12, 2023
@andrii-i
Copy link
Contributor Author

@jtpio I tried to access app.serverapp.extension_manager.extensions in notebook/app.py and it works, thanks for the suggestion

@jtpio
Copy link
Member

jtpio commented May 17, 2023

@andrii-i since the default is now a button we'll have to update the reference snapshots for the UI tests.

Thinking we could also expand the UI tests to cover the case when nbclassic is installed or not (but could be done in a separate PR)

@andrii-i andrii-i force-pushed the detect_nbclassic_dynamically branch from 2acb833 to 28aba34 Compare May 18, 2023 01:31
@andrii-i andrii-i force-pushed the detect_nbclassic_dynamically branch from b7996d1 to 4cc542c Compare May 18, 2023 23:45
@andrii-i
Copy link
Contributor Author

Failing test seems to be flakey. It passed in previous run, I can't reproduce it locally or on Gitpod

@andrii-i
Copy link
Contributor Author

Also after notebook is opened in JupyterLab shell when nbclassic is installed, there is still only one button
Current

@jtpio
Copy link
Member

jtpio commented May 22, 2023

Also after notebook is opened in JupyterLab shell when nbclassic is installed, there is still only one button

Ah right, this is likely because the page config will only be passed to the frontend when served from the notebook application. Wondering if a better could then to pass a parameter via a server extension that would be distributed via the lab extension.

@andrii-i
Copy link
Contributor Author

andrii-i commented May 24, 2023

@tonyfast tagging you to follow up on a discussion during todays JupyterLab call about displaying dropdown menu in cases where there is more than 1 option in "Open in..." menu (used to be called "Interface" menu) and a button otherwise. Please see these threads for a previous discussion on the topic: #6746 (comment), #6866 (comment). Bottomline: we don't expect users to see this UI in its different variants often (and never at once) so having extra friction of dropdown menu with only 1 option seems to be not worth it. Would be interested to hear your opinion.

@tonyfast
Copy link
Collaborator

originally, i thought it didn't make sense to switch the semantics of the UI. now that i see it is a feature being degraded and deprecated then switching semantics makes sense to me. they actually are different things now. steady on. good stuff. thanks for including me.

@andrii-i
Copy link
Contributor Author

@tonyfast Thank you for giving this a look and providing feedback

@andrii-i andrii-i force-pushed the detect_nbclassic_dynamically branch from 0057146 to b48d4c8 Compare June 1, 2023 22:28
notebook/app.py Outdated Show resolved Hide resolved
@andrii-i andrii-i force-pushed the detect_nbclassic_dynamically branch from f798f04 to 936f647 Compare June 8, 2023 20:56
@andrii-i andrii-i requested a review from jtpio June 8, 2023 21:13
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 92f2336 into jupyter:main Jun 9, 2023
@jtpio jtpio mentioned this pull request Jun 9, 2023
2 tasks
@andrii-i
Copy link
Contributor Author

andrii-i commented Jun 9, 2023

Thank you @jtpio

@andrii-i andrii-i deleted the detect_nbclassic_dynamically branch June 9, 2023 06:08
@andrii-i andrii-i changed the title Add option to open a notebook in NbClassic if it is installed; show "Open in..." dropdown menu if there are multiple options, show single button otherwise Add option to open a notebook in NbClassic if it is enabled; show "Open in..." dropdown menu if there are multiple options, show single button otherwise Jun 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants