-
Notifications
You must be signed in to change notification settings - Fork 840
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
Beta badge inner visual alignment #8255
base: main
Are you sure you want to change the base?
Conversation
Hey team, before reviewing from a technical side I'd like to ask somebody from design side to check the visual part and my considerations. @MichaelMarcialis @ryankeairns what are your thoughts? |
I think this looks great visually. It's a small but noticeable improvement. |
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
Minute alignment is tricky as browsers handle sub-pixels differently. One browser might round a 0.4px offset to 0.5px, another to 0 or 1px or not round at all. It won't always look as expected. See this comparison:
Screen.Recording.2025-01-07.at.14.49.05.movScreen.Recording.2025-01-08.at.10.20.04.mov
Screen.Recording.2025-01-07.at.14.49.40.movScreen.Recording.2025-01-08.at.10.18.29.movBased on this, I think it doesn't make sense to try adjust based on sub-pixels. According to these changes we could just add |
m: css` | ||
font-size: ${euiFontSizeFromScale('xs', euiTheme)}; | ||
line-height: ${euiTheme.size.l}; | ||
line-height: ${mathWithUnits(euiTheme.size.l, (x) => x * 0.96)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not a fan of using magic numbers or static values here 🙈
I do like that you're adding a height to the badge and with that the line-height is not really that important anymore, we could just use the same height.
What I'm thinking
// simplified code
const badgeSizes = {
m: euiTheme.size.l,
s: mathWithUnits(euiTheme.size.base, (x) => x * 1.25),
};
euiBetaBadge: {
m: css`
line-height: ${badgeSizes.m};
`,
s: css`
line-height: ${badgeSizes.s};
`,
...
badgeSizes: {
default: {
m: `
${logicalCSS('height', badgeSizes.m)}
`,
s: `
${logicalCSS('height', badgeSizes.s)}
},
}
}
@mgadewoll - following up on your comment regarding various browsers: Badges are used so extensively across our UIs that this minute change may make a bigger difference across the board. |
@kyrspl I would vote against it. Mainly because we don't even know which browser versions handle sub-pixel in what way (a new version might introduce changes) or how they handle font rendering. While I understand it's not looking perfect, I'm wondering if it's mostly something our eyes notice more than a users' 😅 |
> a new version might introduce changes |
Summary
EuiBetaBadge
looks slightly unbalanced in terms of text baseline: the label with uppercase letters by nature feels like being shifted a bit more towards the top of the badge, so I pulled it down very slightly.