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

Disable size containment for split handle #560

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

krassowski
Copy link
Member

@krassowski krassowski added the bug Something isn't working label Mar 18, 2023
@krassowski krassowski added this to the Lumino 2 milestone Mar 18, 2023
@@ -18,6 +18,8 @@

.lm-SplitPanel-handle {
z-index: 1;
/* disable size containment as the handle overhangs another div */
contain: style!important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
contain: style!important;
contain: style !important;

Unless it's meaningful to have style!important as one contiguous token, adding a space would make this more readable.

Choose a reason for hiding this comment

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

Is using !important here OK because essentially we never want the style to be overwritten by anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good question. In JupyterLab it would be ok with the logic "we are overriding lumino and changing widget setting to not use strict containment in JS is not worth the trouble". But here we are modifying core lumino so maybe another fix by not adding contain: strict would be better. I will need to check if we can do so without breaking API.

Copy link
Member Author

Choose a reason for hiding this comment

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

So instead of adding this style we should just remove this line:

handle.style.contain = 'strict';

Thanks for questioning me - the identification of the problem was right but solution was wrong, another reason not to make PRs at 3am even if those are as simple as 2 lines :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Or rather we should change strict to style here.

Copy link

@andrii-i andrii-i Mar 18, 2023

Choose a reason for hiding this comment

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

No problems! I've searched for !important in Lumino before asking and it is used in couple of places in the codebase. Thanks for fix and explanations

Copy link

@andrii-i andrii-i left a comment

Choose a reason for hiding this comment

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

Fixes the problem, side panel "split handle" now behaves in line with previous versions. Below is comparison of 3.2.9 where problem is not yet present (JupyterLab Light theme) vs current head-of-master with proposed fix applied directly to split handle css (JupyterLab Dark theme)

drag_zone

@andrii-i
Copy link

Let's merge this (I don't have access / rights to merge)

@andrii-i
Copy link

@krassowski wanted to ask what needs to be done to put lumino with #560 merged into jupyterlab? Is it something like lumino new release --> update jupyterlab dependencies?

@krassowski
Copy link
Member Author

krassowski commented Mar 21, 2023

Yes. We have a few another PR on lumino which could be included in a new release.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Left panel and right panel resizer is too small to use comfortably
3 participants