jupyterlab / lumino

Lumino is a library for building interactive web applications
https://lumino.readthedocs.io/
Other
635 stars 126 forks source link

Remove the overflow rule on widgets #715

Closed fcollonval closed 3 months ago

fcollonval commented 4 months ago

This removes the overflow: hidden CSS rule on widget.

The rationale behind this change is that

It breaks the ability to have sub items overflow for example one having custom list picker or context menus in cell outputs.

fcollonval commented 3 months ago

Thanks for the review @krassowski This is highly surprising. I definitely need to look at that.

Would it still work if we made the change here but restored this rule in JupyterLab codebase?

Do you mean restoring it where we see changes or overall?

krassowski commented 3 months ago

Do you mean restoring it where we see changes or overall?

I was thinking about something more general like .jp-ThemedContainer .lm-Widget { overflow: hidden } but I do not quite follow what use case you are trying to solve here so not sure if that PR would be still useful for you then

fcollonval commented 3 months ago

I do not quite follow what use case you are trying to solve here so not sure if that PR would be still useful for you then

Unfortunately that will indeed make this PR not useful. The main issue is when integrating a library that does not support mechanism like react portal to attach out of the flow elements outside of the local dom tree.

fcollonval commented 3 months ago

That can be fixed by removing the following rule in JupyterLab:

.jp-SidePanel {
    /* overflow-y: auto; */
}

I saw also it will require some changes in the filebrowser styling:

.jp-FileBrowser .jp-SidePanel-content {
    overflow: hidden;
}

.jp-FileBrowser-Panel {
    overflow: hidden;
}

.jp-DirListing {
    overflow: hidden;
}

Do you think this would be acceptable?

krassowski commented 3 months ago

These would be good, but I guess we should also test with as many extensions as possible to see what else might be affected.

What about more granular scoping .jp-MainAreaWidget .lm-Widget { overflow: hidden } and similar? I do not insist that we need this in the future, just trying to think of ways to make this change as non-breaking for extension authors as possible.

fcollonval commented 3 months ago

These would be good, but I guess we should also test with as many extensions as possible to see what else might be affected.

:+1:

What about more granular scoping .jp-MainAreaWidget .lm-Widget { overflow: hidden } and similar? I do not insist that we need this in the future, just trying to think of ways to make this change as non-breaking for extension authors as possible.

We could add something like that - it could indeed mitigate issues.

fcollonval commented 3 months ago

@krassowski

I tried a couple of extensions[^1] and did not see regressions:

image

[^1]: ipywidgets jupyterlab-git jupyterlab-lsp jupyterlab-geojson jupyterlab-kernelspy jupyterlab-github lckr_jupyterlab_variableinspector jupyterlab-unfold jupyterlab-spreadsheet-editor jupyterlab-search-replace

fcollonval commented 3 months ago

if it is ok for you - I can merge and release lumino to open a PR on Lab

fcollonval commented 3 months ago

this needs more testing downstream and if we find any breakages which cannot be fixed before 4.3 is out we would want to (temporarily) revert this (possibly on lab side).

I agree with you - and yes it will be better to revert it only on lab if required.