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

Default to the full windowing mode #7321

Merged
merged 17 commits into from May 13, 2024
Merged

Default to the full windowing mode #7321

merged 17 commits into from May 13, 2024

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Apr 11, 2024

Fixes #7231
Fixes #7318
Closes #7301

image

notebook-windowing-mode-full.webm

@jtpio jtpio added this to the 7.2.0 milestone Apr 11, 2024
Copy link
Contributor

Binder 👈 Launch a Binder on branch jtpio/notebook/fix-full-css

@jtpio
Copy link
Member Author

jtpio commented Apr 11, 2024

cc @afshin for review as the author of the upstream PR: jupyterlab/jupyterlab#15533

Not sure why but there seems to be some logic setting a width and padding-right on the notebook widget, which conflict with the styles applied here in the Notebook CSS.

I'm not super happy with these !important to get around it, so if you have any suggestion (or if there is a way to fix this upstream) that would be appreciated, thanks!

@jtpio jtpio added the bug label Apr 11, 2024
@jtpio
Copy link
Member Author

jtpio commented Apr 11, 2024

I'm not super happy with these !important to get around it, so if you have any suggestion (or if there is a way to fix this upstream) that would be appreciated, thanks!

Maybe we could live with it for now, as we are still defaulting to defer in Notebook for now (#7318).

@krassowski
Copy link
Member

How to test this on Binder? Each time I try to switch setting to "full" when I reload the notebook it switches back to defer. It looks like setting switching code will make it hard for users to toggle the mode, so I am now less convinced that this is a good idea (with the current implementation).

@jtpio
Copy link
Member Author

jtpio commented Apr 11, 2024

It looks like setting switching code will make it hard for users to toggle the mode, so I am now less convinced that this is a good idea (with the current implementation).

Yes indeed. Ideally we would be able to temporarily default the windowing mode to defer as suggested in #7318. But it looks like using settings transform to do this might not be possible at the moment, so it's not clear what the best path forward is.

Of note there is a similar issue with the defaults used in the file browser (checkboxes on, notebook sorted first), which affect JupyterLab.

@krassowski
Copy link
Member

Another idea, can notebook server extension inject something into overrides.json?

@jtpio
Copy link
Member Author

jtpio commented Apr 11, 2024

Probably. But it might be a bit odd to go down that way, because there is already a way for setting the windowing mode (via the settings). So that would still be a workaround.

Otherwise, maybe there should be a programmatic way to more dynamically set the windowing mode, similar to this request for more easily customizing the file browser: jupyterlab/jupyterlab#14623.

Providing a custom notebook factory in Notebook might also work, but it sounds a bit overkill and would lead to duplicating upstream code from JupyterLab, just to be able to change one default.

@krassowski
Copy link
Member

Not sure why but there seems to be some logic setting a width and padding-right on the notebook widget, which conflict with the styles applied here in the Notebook CSS.

I'm not super happy with these !important to get around it, so if you have any suggestion (or if there is a way to fix this upstream) that would be appreciated, thanks!

While I understand that the !important directive is not pretty I am afraid there is no easy fix for this on JupyterLab side. We spent a lot of time looking at a solution which would work across browsers with @afshin and it was hard to get it right and came to the conclusions that the width and padding have to be computed programmatically.

Now, there is always a way to create a new <style> tag and update it programatically, and just flick a class. This would add a bit more complexity on the lab side though. Is this a solution that would help you, or do you think that sticking to two !important directives is ok?

@jtpio
Copy link
Member Author

jtpio commented Apr 17, 2024

bot please update playwright snapshots

@jtpio jtpio closed this Apr 17, 2024
@jtpio jtpio reopened this Apr 17, 2024
@jtpio
Copy link
Member Author

jtpio commented Apr 17, 2024

Also not sure where we got it from, but it looks like the Notebook now shows a scrollbar by default even when there is a single cell only. This seems to be the case in Notebook 7.1.x too, so not related to this PR in particular:

notebook-default-scrollbar.webm

@jtpio
Copy link
Member Author

jtpio commented Apr 17, 2024

Also not sure where we got it from, but it looks like the Notebook now shows a scrollbar by default even when there is a single cell only. This seems to be the case in Notebook 7.1.x too, so not related to this PR in particular:

Should be fixed by #7327

@jtpio jtpio marked this pull request as ready for review April 17, 2024 16:54
@jtpio
Copy link
Member Author

jtpio commented Apr 18, 2024

Looks like there is a some growing padding at the bottom of the last cell when adding new cells:

full-last-cell-extra.webm

@jtpio
Copy link
Member Author

jtpio commented Apr 18, 2024

Looks like there is a some growing padding at the bottom of the last cell when adding new cells:

This seems to be the same issue as originally reported in #6855 (comment).

@afshin @krassowski or anyone who has worked on the notebook windowing mode, would there be a way to fix or work around that?

@jtpio
Copy link
Member Author

jtpio commented Apr 19, 2024

This seems to be the same issue as originally reported in #6855 (comment).

It looks like this might be caused by the fixed height set on jp-WindowedPanel-inner:

image

Also cc @fcollonval who may know of a fix or workaround.

@jtpio
Copy link
Member Author

jtpio commented Apr 19, 2024

OK gonna work around that for now in #7335, by sticking to defer in Notebook until the issue above is resolved.

@jtpio jtpio marked this pull request as draft April 19, 2024 16:49
@jtpio jtpio changed the title Fix CSS for full windowing mode Default to the full windowing mode Apr 24, 2024
@jtpio
Copy link
Member Author

jtpio commented May 10, 2024

One consequence of this CSS change is the blue border not encompassing the whole cell anymore:

Before After
image image

@jtpio
Copy link
Member Author

jtpio commented May 10, 2024

bot please update playwright snapshots

@jtpio
Copy link
Member Author

jtpio commented May 10, 2024

bot please update playwright snapshots

@jtpio
Copy link
Member Author

jtpio commented May 12, 2024

bot please update playwright snapshots

@jtpio jtpio closed this May 13, 2024
@jtpio jtpio reopened this May 13, 2024
@jtpio
Copy link
Member Author

jtpio commented May 13, 2024

bot please update playwright snapshots

@jtpio jtpio closed this May 13, 2024
@jtpio jtpio reopened this May 13, 2024
@jtpio jtpio marked this pull request as ready for review May 13, 2024 08:30
@jtpio
Copy link
Member Author

jtpio commented May 13, 2024

OK looks like we can now get this one in, and align with the JupyterLab default.

@jtpio jtpio merged commit f5d8aea into jupyter:main May 13, 2024
31 checks passed
@jtpio jtpio deleted the fix-full-css branch May 13, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix the default notebook windowing mode CSS issue when the Notebook windowing mode is set to full
2 participants