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

fix(list-item): Make width of caret icon consistent with selection icon #7862

Closed
wants to merge 1 commit into from

Conversation

driskull
Copy link
Member

@driskull driskull commented Sep 21, 2023

Related Issue: None

Summary

  • Currently, the hit area for the expand/collapse of a list item is a bit small. (18px)
  • This adds padding consistent with the selection icon

Current

image

New

image

@driskull driskull self-assigned this Sep 21, 2023
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Sep 21, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed bug Bug reports for broken functionality. Issues should include a reproduction of the bug. labels Sep 21, 2023
@driskull driskull marked this pull request as ready for review September 21, 2023 23:36
@driskull driskull requested a review from a team as a code owner September 21, 2023 23:36
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Code changes LGTM!

@SkyeSeitz @ashetland Could you give this a looksie? 🔍👁️‍🗨️👄👁️‍🗨️

@ashetland
Copy link

This seems to conflict a bit with what I put together for issue #7100 slated for December (Figma file).

@driskull
Copy link
Member Author

This can wait for December if necessary.

Looking at the design, it would be nice if the design included the hit area for the caret, selection, etc. It looks like its just putting padding in-between the icons but not really showing each icon's hit area. Would the icon include 6px padding on each side?

@driskull
Copy link
Member Author

I guess for this release I was hoping to improve the hit area of the caret without messing with the other paddings. Can we do that?

@ashetland
Copy link

It looks like its just putting padding in-between the icons but not really showing each icon's hit area.

Great catch! Yep, those designs will be revised to include hit areas. I actually started looking into that for #6700, but stopped because it wasn't relevant to the issue 🤦‍♂️. That Figma file also calls for changing the caret to a chevron for system consistency.

Due to all the related issues, it might be worth holding off for now? Merging this would up the space between the chevron and the next element to 2rems which is a lot and doesn't improve the visual hierarchy when nesting.

@ashetland
Copy link

Would the icon include 6px padding on each side?

That sounds right, but I recall some of the paddings getting tweaked depending on whether or not there was nesting.

@driskull
Copy link
Member Author

Yeah lets just close this and wait for december.

@driskull driskull closed this Sep 22, 2023
@driskull driskull deleted the dris0000/list-fix-open-caret-width branch September 25, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants