fix: align banner and toast icons - #726
Conversation
commit: |
Docs PreviewCommit: |
|
/bonk review and approve this approach |
Visual Regression Report — 15 changed, 28 unchanged15 screenshot(s) with visual changes: Button / Loading State1 px (0%) changed
Dialog / Dialog Basic474 px (0.54%) changed
Dialog / Dialog Alert1,169 px (1.32%) changed
Dialog / Dialog Confirmation1,088 px (1.23%) changed
Dialog / Dialog With Select130 px (0.15%) changed
Dialog (Open)0 px (0%) changed
Select / Select Basic859 px (0.97%) changed
Select / Select Sizes170 px (0.1%) changed
Select / Select Without Label721 px (0.81%) changed
Select / Select With Description1,688 px (1.62%) changed
Select / Select With Error1,730 px (1.66%) changed
Select / Select Placeholder237 px (0.27%) changed
Select / Select With Tooltip853 px (0.96%) changed
Select / Select Loading0 px (0%) changed
Select (Open)1,082 px (0%) changed
28 screenshot(s) unchanged
Generated by Kumo Visual Regression |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is clean and consistent across both Banner and Toast. Using a line-height-sized flex container with is a modern, minimal way to keep icons aligned to the first line without shrinking when text wraps. Tests cover the new behavior, and the docs preview is a nice addition.
There was a problem hiding this comment.
The approach is clean and consistent across both Banner and Toast. Using a line-height-sized flex container with flex-none is a modern, minimal way to keep icons aligned to the first line without shrinking when text wraps. Tests cover the new behavior, and the docs preview is a nice addition.
|
✅ Approved PR #726 with the following comment:
Summary of the review:
|
| it("aligns icons to the first text line without shrinking", () => { | ||
| render( | ||
| <Banner | ||
| size="sm" | ||
| icon={<svg data-testid="icon" className="custom-icon" />} | ||
| description="A DNS record already exists in this zone and may wrap onto multiple lines." | ||
| />, | ||
| ); | ||
|
|
||
| const icon = screen.getByTestId("icon"); | ||
| const iconClassName = icon.getAttribute("class") ?? ""; | ||
| expect(iconClassName).toContain("size-[1em]"); | ||
| expect(iconClassName).toContain("flex-none"); | ||
| expect(iconClassName).toContain("custom-icon"); | ||
| expect(icon.parentElement?.className).toContain("h-[1lh]"); | ||
| expect(icon.parentElement?.className).toContain("flex-none"); |
There was a problem hiding this comment.
nit: we shouldn't test on the existence of class names, it's brittle
There was a problem hiding this comment.
truth! I will fix this
| ? cloneElement(iconElement, { | ||
| className: cn( | ||
| iconElement.props.className, | ||
| "size-[1em] flex-none leading-snug", |
There was a problem hiding this comment.
I checked downstream Stratus usage and this does have compatibility impact: there are at least 123 Banner usages across 90 files that pass explicit numeric icon sizes, including many 18px, 20px, and 24px icons. Appending size-[1em] overrides those sizes for Phosphor icons (for example, a requested 20px icon becomes the Banner’s inherited 1em size), while custom icon components that do not forward className may retain their original size, so the normalization is inconsistent across the current ReactNode contract.
Is taking ownership of all Banner icon sizing intentional here? If the goal is only alignment/non-shrinking, the outer h-[1lh] flex-none items-center wrapper appears sufficient without cloning or rewriting the supplied node. If normalization is intentional, I think it should be called out as an API/visual behavior change because Stratus currently relies heavily on explicit icon sizing.
There was a problem hiding this comment.
Intentional. Custom sizes downstream is a defect (in my opinion), but point heard about the impact.
3 participants
Summary
Tests
Notes
Full docs lint currently fails on unrelated existing issues in TabsDemo.tsx, ToolbarDemo.tsx, and ChartCard.astro.