Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

Add an 'interface switcher' to the toolbar #158

Merged
merged 6 commits into from
Sep 3, 2021
Merged

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Jun 8, 2021

Fixes #157

image

Is how it looks. Can switch to lab or classic.

@welcome
Copy link

welcome bot commented Jun 8, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2021

Binder 👈 Launch RetroLab on Binder

@yuvipanda
Copy link
Contributor Author

I also wonder if we should trigger a save of the notebook first? Otherwise you might end up not seeing the proper notebook

@jtpio
Copy link
Member

jtpio commented Jun 9, 2021

Thanks!

We still depend on nbclassic at the moment:

"nbclassic~=0.2",

So there could indeed be an interface switcher like this in the notebook toolbar.

I also wonder if we should trigger a save of the notebook first? Otherwise you might end up not seeing the proper notebook

Sounds good 👍

@jtpio
Copy link
Member

jtpio commented Jun 9, 2021

Also I'm wondering whether this plugin could actually go in the lab extension package?

https://github.com/jupyterlab/retrolab/blob/main/packages/lab-extension/src/index.ts

Then it would also be available in the JupyterLab UI:

image

We could then remove the existing RetroLab icon from the notebook toolbar, since this would already be covered by the switch?

image

@yuvipanda
Copy link
Contributor Author

@jtpio so if we move it to the labextension, we'll have to determine current setup (lab or retro) and adjust choices accordingly. That seems reasonable, I'll do that instead.

@jtpio
Copy link
Member

jtpio commented Jun 9, 2021

We could yes, by checking whether there is an IRetroShell or ILabShell (by having them in optional in the plugin).

It would also be fine to have them all show up in both retro and lab to start with.

@yuvipanda
Copy link
Contributor Author

@jtpio I can't seem to figure out how to test this in lab though :(

@yuvipanda
Copy link
Contributor Author

I did refactor the code to suck less, and theoretically added code to behave differently when running in lab vs retro. Any idea how I can test it?

app/index.js Outdated
@@ -87,6 +87,9 @@ async function main() {
require('@retrolab/docmanager-extension'),
require('@retrolab/help-extension'),
require('@retrolab/notebook-extension'),
require('@retrolab/lab-extension').default.filter(({ id }) =>
Copy link
Member

Choose a reason for hiding this comment

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

The lab-extension package is a "normal" JupyterLab 3.0 prebuilt extension:

retrolab/setup.py

Lines 28 to 41 in 752e21f

labext_name = "@retrolab/lab-extension"
lab_extension_dest = os.path.join(HERE, PACKAGE_NAME, "labextension")
lab_extension_source = os.path.join(HERE, "packages", "lab-extension")
# Representative files that should exist after a successful build
jstargets = [
os.path.join(lab_extension_dest, "package.json"),
os.path.join(main_bundle_dest, "bundle.js"),
]
package_data_spec = {PACKAGE_NAME: ["*", "templates/*", "static/**"]}
data_files_spec = [
("share/jupyter/labextensions/%s" % labext_name, lab_extension_dest, "**"),

So having the interface switcher in that package sounded like it would then be loaded automatically in both retro and lab, since retro also supports loading existing prebuilt JupyterLab extensions.

In that case we shouldn't need to manually include the package in this index.js file.

commandLabel: 'Open in RetroLab',
dropdownLabel: 'RetroLab',
urlPrefix: `${baseUrl}retro/tree/`,
current: app.name === 'RetroLab'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could also add ILabShell and IRetroShell as optional dependencies for the plugin, and check whether they are null to know if the current interface is a JupyterLab-like or RetroLab-like interface?

Copy link
Member

Choose a reason for hiding this comment

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

name could indeed be overriden via the page config or directly in JS like here:

https://github.com/jtpio/jupyterlite/blob/a198d902908483f8eea36fecae858cbd14b4a078/app/lab/index.template.js#L176

@jtpio
Copy link
Member

jtpio commented Jun 11, 2021

Thanks Yuvi for the update 👍

Since the lab-extension package is a prebuild extension for JupyerLab, it should normally be loaded automatically in both interfaces.

For real testing, it would be great to start using end-to-end tools like Galata at some point: #135

app/index.js Outdated Show resolved Hide resolved
@yuvipanda
Copy link
Contributor Author

Ah, I'll make these changes :) And to clarify, when I said 'I do not know how to test these' I was refering to not being able to get JupyterLab to reflect my changes at all! So I can't actually see if what I did worked...

@jtpio
Copy link
Member

jtpio commented Jun 11, 2021

It seems to be showing fine in both retro and lab now when testing with the Binder link above:

image

image

Although choosing one of the dropdown options doesn't seem to perform the switch.

Need to check, but maybe you need to run python -m pip install -e . every time for the changes to the lab extension to take effect locally. If so then we should definitely improve that workflow and use the jupyter labextension develop command.

@yuvipanda
Copy link
Contributor Author

@jtpio yay, running pip install -e . worked. The idea is that instead of saying 'Interface' it will show the current interface by default, and selecting another will switch to that.

I'll change the way to detect which UI we are running by using ILabShell instead of the .name

@yuvipanda
Copy link
Contributor Author

@jtpio gentle bump - anything else needed here?

@jtpio
Copy link
Member

jtpio commented Jun 22, 2021

Thanks Yuvi for the update.

Checking with the Binder link above, it looks like choosing other interfaces has no effect?

interface-switch-binder.mp4

And it seems to be defaulting to JupyterLab.

@jtpio
Copy link
Member

jtpio commented Jun 22, 2021

But using the commands from the command palette works fine.

@yuvipanda
Copy link
Contributor Author

oh interesting - it works for me. I'll investigate it this week.

@jtpio jtpio added this to the 0.3.0 milestone Sep 1, 2021
@jtpio jtpio added the enhancement New feature or request label Sep 3, 2021
@jtpio
Copy link
Member

jtpio commented Sep 3, 2021

@yuvipanda I pushed an update in 3f49eea to show the appropriate notebook toolbar buttons depending on the current interface:

interface-switch-buttons.mp4

Using buttons instead of a dropdown to help simplify the state management.

If you want to try it on Binder:

https://mybinder.org/v2/gh/yuvipanda/retrolab/open-in?urlpath=retro/tree

If it looks good to you we can merge and ship it right after in 0.3.4.

@yuvipanda
Copy link
Contributor Author

Awesome! This looks great to me, thank you for putting effort into it!!!

@jtpio
Copy link
Member

jtpio commented Sep 3, 2021

Great, thanks for checking it out!

Let's get it in then, I'll cut 0.3.4 right after.

@jtpio jtpio merged commit b9cba83 into jupyterlab:main Sep 3, 2021
@jtpio
Copy link
Member

jtpio commented Sep 3, 2021

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple button to switch back to classic notebook
2 participants