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

Update to the latest Lumino #16804

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Sep 23, 2024

References

Extract the Lumino update from #16794 to another PR, to avoid noise in #16794 and fix potential issues with this update not necessarily related to #16794.

Closes #16674

Code changes

  • Update @lumino packages
  • Fix accessing the app _plugins in the UI tests
  • Fix overflow: hidden CSS rule for menus

User-facing changes

None?

Backwards-incompatible changes

None

Copy link

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

@jtpio
Copy link
Member Author

jtpio commented Sep 23, 2024

Adding to 4.3.0 as it would be great to have to unblock #16794 and other streams of work.

@krassowski
Copy link
Member

What do you think about releasing another beta before merging this one? I know it can be annoying as it can cause merge conflicts, but my reasoning is that this will allow to narrow down any regressions by installing different versions?

@jtpio
Copy link
Member Author

jtpio commented Sep 26, 2024

Yeah it would be great to have this Lumino update out soon so it can be tested with extensions and downstream in Notebook 7.

But then that would mean releasing now, merging this PR, and making another right after (to not wait too long), which can also be tedious.

@krassowski
Copy link
Member

krassowski commented Sep 27, 2024

When I reviewed jupyterlab/lumino#715 I noted that there is a regression with sidebar resizing behaviour, was this addressed? I do not see the changes mentioned in jupyterlab/lumino#715 (comment) in the diff but maybe it was addressed differently?

But then that would mean releasing now, merging this PR, and making another right after (to not wait too long), which can also be tedious.

Ok, I will do that - I will take care of resolving conflicts here and making two releases.

@jtpio
Copy link
Member Author

jtpio commented Sep 27, 2024

When I reviewed jupyterlab/lumino#715 I noted that there is a regression with sidebar resizing behaviour, was this addressed? I do not see the changes mentioned in jupyterlab/lumino#715 (comment) in the diff but maybe it was addressed differently?

Ah maybe this is not covered by this PR then, would need to double check.

The menu overflow issues were caught by the UI tests and should now be fixed though.

@jtpio
Copy link
Member Author

jtpio commented Sep 27, 2024

Just checked from this PR and there does seem be an issue when resizing the sidebar:

jupyterlab-lumino-sidebar-resize-issue.webm

@krassowski
Copy link
Member

@jtpio rebased to resolve conflicts after release - do you want to take it from here to fix the sidebar styling?

@jtpio
Copy link
Member Author

jtpio commented Sep 30, 2024

Thanks @krassowski for helping with the rebase and the conflicts!

I just pushed a commit to include the suggestions from jupyterlab/lumino#715 (comment), which seems to be fixing the sidebar issues. Let's see if CI still passes.

@krassowski
Copy link
Member

Should we also add an entry about this in the extension migration guide, ideally with instructions how to restore old behaviour in extensions by adding overflow: hidden?

@jtpio
Copy link
Member Author

jtpio commented Oct 2, 2024

Thanks for the suggestion 👍

Added in 39fd022.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

LGTM, just tiny suggestion on docs - I think we can continue testing this as beta3 :)

docs/source/extension/extension_migration.rst Outdated Show resolved Hide resolved
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment