Skip to content

refactor!: replace abbr SVG with <div> element #9330

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vursen
Copy link
Contributor

@vursen vursen commented May 28, 2025

Description

Replaces the avatar's abbr SVG with a <div> element, following up on #9316, which made the same change for the icon SVG.

This also fixes the issue of abbr rendering with the wrong color in the new base styles:

Before After
localhost_8000_dev_avatar html (1) localhost_8000_dev_avatar html

Warning

This change may be breaking, as font-size on [part=abbr] now behaves differently due to removal of SVG scaling.

Type of change

  • Refactor

@vursen vursen force-pushed the refactor_replace-abbr-svg-with-div branch 4 times, most recently from ee4c445 to ae6bc64 Compare May 28, 2025 14:14
@vursen vursen force-pushed the refactor_replace-abbr-svg-with-div branch 7 times, most recently from 98edddc to d1e0e1b Compare May 29, 2025 06:07
@vursen vursen force-pushed the refactor_replace-abbr-svg-with-div branch from d1e0e1b to b1ec14e Compare May 29, 2025 06:12
Copy link

@vursen vursen requested review from jouni and web-padawan May 29, 2025 06:28
}

:host([theme~='xlarge']) [part='abbr'] {
font-size: 2.5em;
Copy link
Contributor Author

@vursen vursen May 29, 2025

Choose a reason for hiding this comment

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

note: These values were originally calculated with SVG scaling in mind, so they are no longer valid. However, I was able to achieve a relatively close result using font-size: round(up, var(--_avatar-size) * 0.4, 3px) @jouni let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

It’s a bit complex at first glance, but who am I to criticize anyone’s CSS of that, when I’m often the one coming up with very elaborate constructs 😆

I didn't remember to test it locally, but am I right to assume that the font size adjusts with 3px increments, i.e., it’s always divisible by 3?

Copy link
Contributor Author

@vursen vursen May 30, 2025

Choose a reason for hiding this comment

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

It’s a bit complex at first glance, but who am I to criticize anyone’s CSS of that, when I’m often the one coming up with very elaborate constructs

Yeah, it might not be immediately clear...

var(--_avatar-size) * 0.4 could be replaced with 0.4sqh assuming we make avatar a CSS container, but it seemed to me a good idea to avoid CSS containers when values are already available.

I didn't remember to test it locally, but am I right to assume that the font size adjusts with 3px increments, i.e., it’s always divisible by 3?

Yes, that's right.

@jouni
Copy link
Member

jouni commented May 30, 2025

I don't like that now the abbreviation isn't exactly centered, when we rely on the line-height to center it. But that depends on the font, how the characters are centered in the line box.

I’d rather make the avatar host a flex container and use align-items: center and then line-height: 0 or text-box: cap alphabetic (only supported in Chrome and Safari 18.2) to properly center the text.

Was there some issue using line-height: 0 that I already forgot?

@vursen vursen marked this pull request as draft June 19, 2025 11:13
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