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

Increase notebook-cell margin in side-by-side mode #11539

Merged
merged 3 commits into from Dec 1, 2021

Conversation

jess-x
Copy link
Contributor

@jess-x jess-x commented Nov 25, 2021

References

Fixes #11376

Code changes

When in side-by-side mode, increase top/bottom margins to 3em & left/right margins to 5%

User-facing changes

Before:

Screen.Recording.2021-11-24.at.4.54.04.PM.mov

After:

Screen.Recording.2021-11-24.at.5.07.28.PM.mov

Backwards-incompatible changes

@jupyterlab-probot
Copy link

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

@github-actions github-actions bot added Design System CSS pkg:notebook tag:CSS For general CSS related issues and pecadilloes labels Nov 25, 2021
@jess-x jess-x requested a review from echarles November 25, 2021 01:20
@echarles
Copy link
Member

This looks better with the margins. 2 comments:

  1. I see 2 collapsers (the blue vertical lines). I am no more sure if this was already there before. Is this intented?
  2. You have set the margin to margin: 3em;. Have you tried a '%' instead of a fixed value? Just asking to see if we could have a margin depending on the actual width of the notebook panel.

Screenshot 2021-11-25 at 07 50 08

@jess-x jess-x added this to the 4.0 milestone Nov 26, 2021
@jess-x
Copy link
Contributor Author

jess-x commented Nov 26, 2021

Screen Shot 2021-11-26 at 2 03 01 PM
This is how 2% notebook-cell margins look on my screen. I cannot actually decide between this and 3em by eyeballing the minimal difference they have on my screen, but using '%' does make more intuitive sense to me. Thanks @echarles

@jess-x
Copy link
Contributor Author

jess-x commented Nov 26, 2021

I see 2 collapsers (the blue vertical lines). I am no more sure if this was already there before. Is this intended?

Hmm, the left most ones are the input collapsers and the ones on the right side are output collapsers, I believe. Visually they can be confusing, I see. I could address this separately

@echarles
Copy link
Member

Hmm, the left most ones are the input collapsers and the ones on the right side are output collapsers, I believe. Visually they can be confusing, I see. I could address this separately

Yeah, I have been fooled by the input and output collapsers. Let's leave it as they are for now.

@echarles
Copy link
Member

echarles commented Nov 27, 2021

This is how 2% notebook-cell margins look on my screen. I cannot actually decide between this and 3em by eyeballing the minimal difference they have on my screen, but using '%' does make more intuitive sense to me. Thanks @echarles

Prolly just set only margin-left and margin-right instead of margin(margin will set the top and bottom, which we don't need here)

I have tried with margin-left: 5% and margin-right: 5% and sounds to me like giving a better display, especially when you resize the browser to a small viewport.

@jess-x
Copy link
Contributor Author

jess-x commented Nov 28, 2021

Prolly just set only margin-left and margin-right instead of margin(margin will set the top and bottom, which we don't need here)
I have tried with margin-left: 5% and margin-right: 5% and sounds to me like giving a better display, especially when you resize the browser to a small viewport.

I think right now in side-by-side mode, the cells are too clustered together, that's why users are having a harder time focusing outputs with their input cells. I am aware by increasing margin, margin-top and margin-bottom are both increased under the hood. That is an intentional UI/UX decision from me, and I'd argue that's better than just increasing left/right margins. In terms of 2% vs 5%, again I don't have a strong opinion/argument as I don't have access to various screens w/ different sizes. And I think A/B testing that with power users might let us make a more educated decision (if that's possible). With that said, I personally like the current implementation (margin: 2%), but feel free to push to this branch if needed :)

@echarles
Copy link
Member

I think right now in side-by-side mode, the cells are too clustered together,

Thx for the reminder. In that case, increasing top and bottom makes sense.

And I think A/B testing that with power users might let us make a more educated decision (if that's possible).

True. You have spent more time than me trying the different options. 2% was ok for me also.

Let's wait a bit before merging for anyone wiling to give it a try and provide feedback.

@fcollonval
Copy link
Member

I tried it on my ultra wide screen. I would not use a percentage for the vertical margin. Even 2% starts to be a lot. And if you close or not a side panel (see below), this changes the displayed cells although the height did not change. So I would use a constant vertical margin not a variable one.

With filebrowser open Without file browser
side-by-side_ side_by_side_no_filebrowser

@echarles
Copy link
Member

So I would use a constant vertical margin not a variable one.

Makes sense.

@jess-x
Copy link
Contributor Author

jess-x commented Nov 30, 2021

@fcollonval @echarles thanks!! I changed top/bottom margins to 3em, left/right margins to 5%. On my screen, looks like this right now
Screen Shot 2021-11-29 at 7 01 49 PM

@jess-x jess-x added bug and removed enhancement labels Nov 30, 2021
@jtpio
Copy link
Member

jtpio commented Nov 30, 2021

Thanks @jess-x.

left/right margins to 5%

@fcollonval how does the 5% look on the wide display?

@fcollonval
Copy link
Member

@fcollonval how does the 5% look on the wide display?

With sidebar Without sidebar
wide_screen_with_sidebar wide_screen_no_sidebar

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @jess-x

@echarles echarles merged commit 7a2878a into jupyterlab:master Dec 1, 2021
@echarles
Copy link
Member

echarles commented Dec 1, 2021

Thx @jess-x

@echarles
Copy link
Member

echarles commented Dec 1, 2021

@meeseeksdev please backport to 3.2.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Dec 1, 2021
echarles added a commit that referenced this pull request Dec 1, 2021
…539-on-3.2.x

Backport PR #11539 on branch 3.2.x (Increase notebook-cell margin in side-by-side mode)
@fcollonval fcollonval modified the milestones: 4.0, 3.2.x Dec 1, 2021
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 31, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Design System CSS pkg:notebook status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:CSS For general CSS related issues and pecadilloes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Side-by-side rendering feedback
4 participants