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

Restore horizontal scrolling of outputs for Firefox #15171

Merged
merged 14 commits into from Oct 11, 2023

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Sep 28, 2023

References

Fixes #14625

Code changes

It revert the changes done in #11508 to apply them only when printing
It adds the generation of a test notebook for printing

image

User-facing changes

Horizontal scroll on FireFox is restored

Backwards-incompatible changes

None

@jupyterlab-probot
Copy link

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

@fcollonval fcollonval added the bug label Sep 28, 2023
@fcollonval fcollonval added this to the 4.0.x milestone Sep 28, 2023
@fcollonval fcollonval changed the title Fix/14625-scope-css-with-media Restore horizontal scrolling of outputs for Firefox Sep 28, 2023
@krassowski krassowski self-assigned this Oct 8, 2023
@krassowski
Copy link
Member

krassowski commented Oct 8, 2023

It looks this also breaks the mobile layout, or at least a test for it:

Screenshot from 2023-10-08 16-40-37

And Vega outputs positioning changes:

image

@krassowski
Copy link
Member

krassowski commented Oct 8, 2023

I finished up the fix for the mobile layout and reverted snapshot updated for mobile-layout-jupyterlab-linux.png.

The Vega change is probably expected and a result of:

.jp-RenderedVegaCommon5 {
margin-left: 8px;
margin-top: 8px;
}

and

Note that Firefox disregards the margin-left and margin-right rules because it doesn't apply them to elements with style display: table-cell, like this one. #12378 (comment)

I do not have a strong opinion on spacing so I guess it is fine to keep. I would say that if CI passes this is good to merge.

@krassowski krassowski assigned krassowski and unassigned krassowski Oct 8, 2023
@krassowski krassowski force-pushed the fix/14625-scope-css-with-media branch from f320e14 to cb0a830 Compare October 9, 2023 20:50
@krassowski krassowski force-pushed the fix/14625-scope-css-with-media branch from 8f61152 to 1a1821f Compare October 10, 2023 22:04
Copy link
Member

Choose a reason for hiding this comment

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

@jtpio is this good enough or should we shrink top/bottom margins back to closer match the previous snapshot?

A part of me things that maybe the larger spacing is good for mobile (and possibly even required by accessibility) because the tap targets are easier to separate. I do not have a strong opinion though.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good on the new screenshot. Actually a bit more spacing makes it a bit more pleasant to read it seems.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure there was a specific reason for having the margins very small on mobile in the first place, maybe it was just a side-effect of the table layout.

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
@krassowski
Copy link
Member

bot please update galata snapshots

@github-actions
Copy link
Contributor

Galata snapshots updated.

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.

All 💚 now. Thank you @fcollonval!

@jtpio
Copy link
Member

jtpio commented Oct 11, 2023

Nice, thanks!

@jtpio jtpio merged commit 64e0107 into jupyterlab:main Oct 11, 2023
78 of 79 checks passed
@jtpio
Copy link
Member

jtpio commented Oct 11, 2023

@meeseeksdev please backport to 4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 11, 2023

Something went wrong ... Please have a look at my logs.

It seems that the branch you are trying to backport to does not exist.

@jtpio
Copy link
Member

jtpio commented Oct 11, 2023

@meeseeksdev please backport to 4.0.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Horizontal scrolling unavailable in Firefox
3 participants