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

Reduced padding in cell around code icons in code toolbar #1072

Merged
merged 6 commits into from
Nov 4, 2024

Conversation

srdas
Copy link
Collaborator

@srdas srdas commented Oct 31, 2024

Fixes #1070

Reduces vertical and horizontal padding to 2px

New look:
image

@srdas srdas added the enhancement New feature or request label Oct 31, 2024
@srdas srdas requested a review from dlqqq October 31, 2024 00:09
@srdas srdas self-assigned this Oct 31, 2024
@krassowski
Copy link
Member

@krassowski
Copy link
Member

krassowski commented Oct 31, 2024

I think the change in this PR does not impact the above, but the padding of icons themselves might be too small?

@srdas
Copy link
Collaborator Author

srdas commented Oct 31, 2024

Thanks for noting the accessibility criteria here. Also, I appreciate the link to the standard for target icons. To paraphrase the standard: It would require the icons to be at least 24px in height and width OR if smaller, they should be spaced such that if there were a circle of 24px diameter around each icon, none of the circles would intersect (the "spacing" exception). These criteria are not met (the current icons are 16px in height and width, and the 24px diameter condition is also not met), though as you point out, they are not the subject of this PR.

There are other permitted exceptions. One is that the function can be achieved through a different control on the same page that meets this criterion. Since the action prescribes cut and paste into a code cell, it could definitely be executed through highlighting the code in the chat panel and then cutting and pasting it over into the Jupyter notebook.

The other exceptions do not apply to this case: (i) The code icons are not an inline artifact, (ii) they are not driven by a user agent that is not under software control, and (iii) the size of the icons is not legally mandated to be smaller than standard.

There are several ways to proceed:

  1. We accept that since the icon actions are possible via a simple cut and paste, we are covered under a mandated exception.
  2. We increase the icon dimensions from the current $16\mbox{px}^2$ to $24\mbox{px}^2$. I tried this and it looks huge and is not aesthetically appealing.
  3. We increase the spacing between the icons to 8px, so that the spacing exception is met. This is shown in the standard:
image

Now, this PR only amends the padding around the toolbar and does not violate any additional conditions. Vertical clearance is within standard as below:
image

@ellisonbg @krassowski @dlqqq @JasonWeill @3coins @andrii-i -- What do you think? If the standard Jupyter Lab spacing between icons is already >= 8px, then it is not an exception.
@krassowski TY for bringing attention to this accessibility standard.

@srdas
Copy link
Collaborator Author

srdas commented Nov 1, 2024

Adding an extra icon of 8px (height and width) next to the standard icons of 16px indicates that the default Jupyter icon spacing seems to be at least 8px, suggesting that the icons are properly spaced apart based on the standard (i.e., already adhere to the spacing exception).
image
The second icon is 8px width and is smaller than the space between the other icons.

@dlqqq
Copy link
Member

dlqqq commented Nov 1, 2024

@srdas

Vertical clearance is within standard as below:

The image you included seems to imply we only need 24px of horizontal space between the center of each icon, since there are no clickable elements above or below the cell toolbar.

Given that the icons are 16px in both dimensions, we need minimum 4px of horizontal padding. However, we can leave the vertical padding unchanged at 2px to preserve vertical screen space (the original intent behind this PR).

Can you update this PR with these suggested changes and post a screenshot for us here?

@srdas
Copy link
Collaborator Author

srdas commented Nov 1, 2024

@ellisonbg @dlqqq Added some horizontal space to make the icons a little more spaced apart to resemble the notebook code block toolbar. Also set the top and bottom spacing to 2px.

Old spacing:
image

New spacing:
image

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@srdas Adding right padding to each icon causes the icon background to be oval instead of circular on hover. The tooltip is also displaced to the right.

Screenshot 2024-11-01 at 5 32 59 PM

Instead of adding 8px padding to the icon elements, can you add 8px right margin to the TooltippedIconButton elements (which are the direct parents of the icon elements here)?

TooltippedIconButton accepts the sx prop used by MUI components, which allows you to specify right margin: https://mui.com/system/getting-started/the-sx-prop/

@srdas
Copy link
Collaborator Author

srdas commented Nov 4, 2024

Implemented the feature with sx prop and it is seen below:
image

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

Can the right margin not be applied to the right-most cell toolbar button, to keep it aligned to the right?

@srdas
Copy link
Collaborator Author

srdas commented Nov 4, 2024

Can the right margin not be applied to the right-most cell toolbar button, to keep it aligned to the right?

Good point. Will adjust the left margin as that does it cleanly for all four icons. As shown here:
image

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

Great work! Verified changes locally.

Screenshot 2024-11-04 at 9 16 52 AM

@srdas srdas merged commit 49709dc into jupyterlab:main Nov 4, 2024
10 checks passed
@srdas
Copy link
Collaborator Author

srdas commented Nov 4, 2024

@meeseeksdev please backport to v3-dev

meeseeksmachine pushed a commit to meeseeksmachine/jupyter-ai that referenced this pull request Nov 4, 2024
srdas added a commit that referenced this pull request Nov 4, 2024
…toolbar (#1084)

Co-authored-by: Sanjiv Das <srdas@scu.edu>
@dlqqq
Copy link
Member

dlqqq commented Nov 4, 2024

cc @jtpio @brichet re: jupyter-chat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code toolbar padding is excessive
3 participants