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

Add active cell border padding, remove double cell padding #7570

Merged
merged 11 commits into from
Feb 4, 2025

Conversation

andrii-i
Copy link
Contributor

@andrii-i andrii-i commented Jan 29, 2025

Add active cell left and right border padding, remove double cell left and right padding.

Code changes

Remove notebook-specific css rules that overwrite css rules inherited from JupyterLab on left and right border padding, cell left and right padding.

User-facing changes

Active cell left and right border padding is added, double padding around cells on left and right is removed.

Screenshots

Before (2nd screenshot is at 400% zoom):
Screenshot 2025-01-28 at 11 54 02 PM

Screenshot 2025-01-29 at 12 20 48 AM

409074777-20c4f5e3-a203-487b-9fe6-b8c58400c04c

After (2nd screenshot is at 400% zoom):
Screenshot 2025-01-28 at 11 52 51 PM

Screenshot 2025-01-29 at 12 18 50 AM

409180849-d0fc3a44-92ae-4083-8a5d-ea76b832a10d

Copy link
Contributor

Binder 👈 Launch a Binder on branch andrii-i/notebook/active-cell-border-padding

@andrii-i andrii-i marked this pull request as ready for review January 29, 2025 17:03
@andrii-i
Copy link
Contributor Author

please update snapshots

@krassowski
Copy link
Member

No idea why action did not pick up your request above. Let me try:

bot please update galata snapshots

@RRosio
Copy link
Collaborator

RRosio commented Jan 30, 2025

Edit: Wow, I had a stale page and I did not see Mike's PR!

Hi @krassowski and @andrii-i, I saw this issue with the bot and tried looking into it. I see that the job was skipped and I thought it might be because of the author_association check that is made. I looked into the collaborators on the repo and I see neither of you are listed, which is strange because I thought you were both previously added (for triage, I believe?)... I can give it a try here to check, and I can add you as collaborators if that sounds good to you both.

bot please update galata snapshots

@RRosio
Copy link
Collaborator

RRosio commented Jan 30, 2025

Sorry for the noise... I see now that Andrii is a member of the Notebook Council team and that Mike's PR adds checks for comment authors

@krassowski
Copy link
Member

krassowski commented Jan 30, 2025

Thanks for the merge @RRosio! Maybe it has to do with write permissions? I do not have them on this repo. Let me try again:

bot please update galata snapshots

@RRosio if this does not get a 👍 from bot, can you try?

@krassowski
Copy link
Member

It worked! https://github.com/jupyter/notebook/actions/runs/13051242848

@krassowski
Copy link
Member

It looks like snapshots are also failing on main, possibly due to release of JupyterLab 4.4.0a3 which includes Console rework:

image

anyways, let's wait and see

@andrii-i
Copy link
Contributor Author

@krassowski @RRosio thank you both for looking into CI failures.

@jtpio
Copy link
Member

jtpio commented Feb 3, 2025

Thanks all.

Updating the branch to check if #7577 helps with the CI failures.

@jtpio
Copy link
Member

jtpio commented Feb 3, 2025

bot please update playwright snapshots

@jtpio jtpio closed this Feb 3, 2025
@jtpio jtpio reopened this Feb 3, 2025
@jtpio jtpio added this to the 7.4.0 milestone Feb 3, 2025
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks @andrii-i for working on this 👍

This change seems to be having the following side-effect when selecting all cells:

image

For reference this is what it looks like with 7.4.0a1 (no left/right margins):

image

Raising this point in case folks have an opinion on which one looks better.

@andrii-i
Copy link
Contributor Author

andrii-i commented Feb 3, 2025

@jtpio Thank you for the looking into this PR and pointing out impact on selecting all cells highlight. Added screenshots to the "Before" and "After" sections of the PR description for more visibility.

This PR removes padding: unset; from WindowedPanel viewport which adds left and right padding back to it. And selecting all cells highlights cells, not a viewport. So impact on selecting all cells highlight makes sense from code changes to design system standpoint, not sure which option looks better but this PR makes this aspect of UI consistent with JupyterLab:
Screenshot 2025-02-03 at 7 52 13 AM

@jtpio
Copy link
Member

jtpio commented Feb 3, 2025

this PR makes this aspect of UI the consistent with JupyterLab:

Right good point. We can likely go with the current state of this PR for consistency then 👍

@andrii-i
Copy link
Contributor Author

andrii-i commented Feb 3, 2025

Other than having consistent user experience, consistent user interface between lab and notebook would also reduce operational load that maintaining different Notebook user interface while JupyterLab is changing generates.

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit aad43d5 into jupyter:main Feb 4, 2025
31 checks passed
@andrii-i
Copy link
Contributor Author

andrii-i commented Feb 4, 2025

@krassowski @RRosio @jtpio Thank you everyone for working on this.

@andrii-i andrii-i deleted the active-cell-border-padding branch February 4, 2025 16:59
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.

Add left and right padding to active cell border
4 participants