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

Changes interface menu to button #6793

Closed
wants to merge 11 commits into from

Conversation

JasonWeill
Copy link
Collaborator

@JasonWeill JasonWeill commented Mar 24, 2023

Fixes #6792, per coordination with @ellisonbg .

Changes the "Interface" dropdown menu back to a button, as it had been before jupyterlab/retrolab#309.

Demo video of switching back and forth:

Screen.Recording.2023-03-23.at.5.49.41.PM.mov

Missing:

  • Getting the button to appear strictly before the kernelName item (in practice, this item is not present yet when I do the check, so I always add the item at the end of the toolbar)
  • Adding context menu items on the notebook context menu

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch JasonWeill/notebook/interface-menu-to-button

@JasonWeill JasonWeill added the bug label Mar 24, 2023
@JasonWeill JasonWeill marked this pull request as ready for review March 24, 2023 01:00
@JasonWeill
Copy link
Collaborator Author

bot please update playwright snapshots

@jtpio
Copy link
Member

jtpio commented Mar 24, 2023

Thanks @JasonWeill for looking into this.

Wondering how that will work if at some point later we would like to allow for opening NBClassic as well? #6746

Would that mean adding a new icon to the toolbar? Potentially this could add some confusion as originally mentioned in jupyterlab/retrolab#256?

cc @yuvipanda who might have an opinion on this (when multiple Jupyter frontends are being deployed together)

@yuvipanda
Copy link
Contributor

The menu was a huge hit with our users, and one of the primary reasons we were able to move the data8 class with 1500+ students away from classic notebook - that they could easily switch back any time if needed. I would <3 if we could keep it that way - nobody (including me!) could make out what the button does or did, and it just looks like a logo.

@JasonWeill
Copy link
Collaborator Author

@yuvipanda The "interface" menu takes up a lot of horizontal space in the toolbar, considering that it only has one option in it. Even if we had two buttons, they would still take up less horizontal space than the interface menu/dropdown does today.

I agree that the buttons in this PR look more like logos than actions. I can ask our UX designers if they can come up with icons that better convey "open this using a different application in a new tab".

@krassowski
Copy link
Member

The "interface" menu takes up a lot of horizontal space in the toolbar, considering that it only has one option in it

  1. Users are free to hide toolbar items as these are customisable.
  2. It would be lovely if some developer time could be put towards making the customisation easier (e.g. work from context menu see Allow to customize toolbar from context menu jupyterlab/jupyterlab#13869)

I can ask our UX designers if they can come up with icons that better convey "open this using a different application in a new tab"

That would be a good thing to do indeed. Some ideas each with distinct pros/cons:

  • use a toggle
  • use "UI" as an abbreviation for user interface
  • move it out of the toolbar altogether and put:
    • next to the trusted indicator
    • next to/below the application logo (I am thinking about something like "Jupyter Notebook [v]" and clicking on [v] expands a menu showing JupyterLab as an option)

@JasonWeill
Copy link
Collaborator Author

@krassowski How about replacing the single icon with a graphic showing both interfaces, with a toggle between them? Clicking on the graphic would open the other interface in a new tab.

Here's a non-functional mockup:

image

@JasonWeill JasonWeill force-pushed the interface-menu-to-button branch from ead6e85 to 801fbfd Compare March 24, 2023 23:11
@krassowski
Copy link
Member

Looks fine; two thoughts:

  • it is not immediately clear that the three elements (two icons and toggle) are part of the same item; adding a shared background or colour or another visual indication could help; shared background on hover would be one option.
  • the colourful icons stand out compared to other icons in toolbar; maybe those should all be greyscale? And/or the inactive one could be toned down/have reduced opacity?

@yuvipanda
Copy link
Contributor

While I understand this text-based toolbar takes a lot of space, I am kinda nervous about the upcoming notebook 7 change. Based on our experience getting both students and instructors to switch successfully to retrolab, knowing that they can easily switch back to classic - even if they never did - was extremely important. May I ask the super obvious text based switcher is left in place at least for one full release, as a trade-off of toolbar space? I think 'switch UI to open same document in a different way' is not particularly easy to recognize in icon form, as that's not a super common UI pattern that exists elsewhere. There isn't enough recognition of the icon differences between Jupyter (the project), the classic notebook, lab, etc for this to work in icon form currently. Please?

@jtpio
Copy link
Member

jtpio commented Mar 28, 2023

Thanks @yuvipanda 👍 I agree the existing dropdown is more explicit and easier to understand which was the original motivation for jupyterlab/retrolab#309.

Also the combination of notebook icon + switch + jupyter icon as proposed in #6793 (comment) almost becomes as large as the "Interface" text of the dropdown. Not sure we save a lot of space there.

Otherwise if the issue is the space taken in the toolbar we could also try to find a shorter label than "Interface". Or make it configurable via the settings.

@JasonWeill JasonWeill marked this pull request as draft March 28, 2023 22:48
@JasonWeill
Copy link
Collaborator Author

Adds a button with a short label and an "open in new" icon to the right of the label.

From notebook to lab:

image

From lab to notebook:

image

@JasonWeill JasonWeill marked this pull request as ready for review March 29, 2023 00:24
@JasonWeill JasonWeill requested a review from ellisonbg March 29, 2023 22:39
@JasonWeill JasonWeill requested a review from afshin March 29, 2023 22:39
@jtpio
Copy link
Member

jtpio commented Mar 30, 2023

As mentioned above, we should likely consider the case of also showing an entry for opening NBClassic (the classic notebook interface): #6746. In the case where JupyterLab 4, Notebook 7 and NBClassic 1 are all installed together on the same Jupyter Server.

For reference there is a gist to try this on Binder: https://gist.github.com/jtpio/35a72862c8be13dee31b61ebac2d9786

Which means a single button in the notebook toolbar will not be enough to open either lab or nbclassic. The existing dropdown still looks like the most natural and less confusing approach I think.

@yuvipanda
Copy link
Contributor

I appreciate you working a lot with us to try different approaches, @JasonWeill!

I concur with @jtpio - the primary use case is being able to switch between three interfaces, not two. I understand that it may be 2 in the future, but I think for broader acceptance of notebook 7, being able to switch back and forth between the three interfaces is very vital. Even if people don't actually use it, there's a lot of resistance to switching that is easily dissolved by 'you can switch back any time you want easily, without having to edit URLs'.

@yuvipanda
Copy link
Contributor

I had not realized that the dropdown was lost when retrolab became notebook v7. I would love for it to be back. As a JupyterHub admin who is going to have to handle this migration, it would make my life a million times easier.

@jtpio
Copy link
Member

jtpio commented Mar 30, 2023

I had not realized that the dropdown was lost when retrolab became notebook v7.

It's still here, but only showing JupyterLab for now because there is no dependency on the classic notebook anymore. Before it was installed by default because of the jupyterlab -> notebook dependency.

So as part of #6746:

  1. either we add a new entry pointing to /nbclassic/notebook, but without depending on nbclassic in notebook. The link might give a 404 if nbclassic is not installed. But would work if nbclassic is installed.
  2. or we add the new entry as mentioned in 1., and also make notebook==7 depend on nbclassic to make sure it's always there.

Maybe option 1.would be enough to start with, and there is still time to consider 2.

@yuvipanda
Copy link
Contributor

I would highly recommend option 2, with a notice period when the automatic dependency would be removed (8.x?). I think this migration is gonna be hard, and the small things we can do to make it easier will have a big payoff.

@jtpio
Copy link
Member

jtpio commented May 5, 2023

Should this be closed now that #6847 has been merged?

@andrii-i
Copy link
Contributor

andrii-i commented May 5, 2023

@jtpio not sure about this PR but please don't resolve #6792 yet as I plan to work on it after modifying server in order to be able to detect nbclassic dynamically

andrii-i added a commit to andrii-i/notebook that referenced this pull request May 12, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request May 18, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request May 18, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request May 18, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request May 18, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 1, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 1, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 2, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 2, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 8, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 8, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 8, 2023
andrii-i added a commit to andrii-i/notebook that referenced this pull request Jun 8, 2023
jtpio pushed a commit that referenced this pull request Jun 9, 2023
…Open in..." dropdown menu if there are multiple options, show single button otherwise (#6866)

* add option to open in nbclassic if installed

* account that getOption() can return truthy 'false'

* immidiately convert nbclassicInstalled to boolean

* Capitalize NbClassic as in PyPI per @JasonWeill

* capitalize commandDescription

* Update packages/lab-extension/src/index.ts

Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>

* Update packages/lab-extension/src/index.ts

Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>

* Update packages/lab-extension/src/index.ts

Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>

* Update packages/lab-extension/src/index.ts

Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>

* Update packages/lab-extension/src/index.ts

Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>

* Update packages/lab-extension/src/index.ts

Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>

* add openInNewIcon based on ##6793 by @JasonWeill

* show single 'open' button if only one option is available

* add button styling

* add css class to toolbarButton

* rename addCommand to addSwitcherCommand

* set page_config["nbclassic_installed"]

* Update snapshots

* fix general.spec.ts lint problem as detected in CI run https://github.com/jupyter/notebook/actions/runs/5008582825/jobs/8976612053?pr=6866

* Use optional chaining instead of non-null assertion to fix lint error https://github.com/jupyter/notebook/actions/runs/5008628805/jobs/8976713072?pr=6866

* add new line to try to fix eslint error

* fix prettier CI errors

* Revert "fix general.spec.ts lint problem as detected in CI run https://github.com/jupyter/notebook/actions/runs/5008582825/jobs/8976612053?pr=6866"

This reverts commit 9f0b544.

* Revert "add openInNewIcon based on ##6793 by @JasonWeill"

This reverts commit eca0a4e.

* use launchIcon instead of openInNewIcon

* fix nbclassic urlprefix

* update general snapshots to account for use of launchIcon

* update mobile snapshots to account for launchIcon

* check if extension is enabled, not installed

* add bundled server extension

* Revert "add bundled server extension"

This reverts commit 7d261db.

* set page_config in initalize_handlers

---------

Co-authored-by: Jason Weill <93281816+JasonWeill@users.noreply.github.com>
@jtpio
Copy link
Member

jtpio commented Jun 9, 2023

Closing as fixed by #6866, thanks!

@jtpio jtpio closed this Jun 9, 2023
@jtpio jtpio added this to the 7.0 milestone Jun 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2024
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.

Replace "Interface" dropdown with button if only one option is present
5 participants