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

Custom icon styling tweaks #1877

Merged
merged 5 commits into from
Feb 28, 2025
Merged

Custom icon styling tweaks #1877

merged 5 commits into from
Feb 28, 2025

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Feb 28, 2025

Follow up to #1853.

Currently, if you run the example there with the shiny logo <img>, you'll get

Screenshot 2025-02-27 at 6 35 54 PM

So, 1cf74c1 sets max-height like we do for svg, etc. which leads to:

Screenshot 2025-02-27 at 6 37 21 PM

Which made me realize that if the image has lots of whitespace, or already something like a border, it's going to look too small/busy.

For this reason, 65d59d2 allow you to put a .border-0 class on the icon container to get rid of the border/border-radius:

Screenshot 2025-02-27 at 6 39 26 PM

@cpsievert cpsievert requested a review from gadenbuie February 28, 2025 00:40
@gadenbuie
Copy link
Collaborator

I'd rather update the example than change the CSS. A lot of what you described was intentional.

I think the shiny image is a bit of a misdirection; the primary use case for raster images is likely to be avatar style images. The escape hatch if you have an icon in an image is to add an .icon class to the img element.

The icon test in the chat playwright tests is a better example; I forgot to return to the example app (where we should use the same image as in the test app or add the .icon class).

@cpsievert
Copy link
Collaborator Author

From a little bit of playing around with other (avatar) images, it doesn't seem all that uncommon for the image content to be clipped by the border-radius on the container. Adding .icon does indeed help, but I still think there are two remaining problems that something like .border-0 could help with:

  1. It doesn't seem that uncommon for avatar images to come with their own border.
  2. 20x20px rendering still feels a bit small for me for non-trivial shapes.

@gadenbuie
Copy link
Collaborator

2. 20x20px rendering still feels a bit small for me for non-trivial shapes.

Yeah, these images shouldn't be 20x20, they should be full bleed. At least that was the intention (and what I see on main).

image

@cpsievert
Copy link
Collaborator Author

they should be full bleed. At least that was the intention (and what I see on main).

Ah, I see where you're coming from now

Copy link
Collaborator

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

nice!

@cpsievert cpsievert merged commit c5d9b9b into main Feb 28, 2025
39 of 41 checks passed
@cpsievert cpsievert deleted the chat-icon-border-0 branch February 28, 2025 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants