Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/align-banner-toast-icons.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@cloudflare/kumo": patch
---

Align Banner and Toast status icons to the first line of wrapping message text and prevent the icons from shrinking.
32 changes: 32 additions & 0 deletions packages/kumo-docs-astro/src/components/demos/BannerDemo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,38 @@ export function BannerWithIconDemo() {
);
}

export function BannerIconAlignmentPreview() {
const message =
"A DNS record for puppies.cloudflare.dev already exists in this zone and must be reviewed before continuing.";

return (
<div className="grid w-full gap-3 md:grid-cols-2">
<div className="space-y-2">
<p className="text-sm font-medium text-kumo-subtle">Before</p>
<div className="flex max-w-72 items-center gap-2 rounded-md bg-kumo-warning-tint px-3 py-2 text-sm text-kumo-warning">
<Warning
weight="fill"
className="size-[1em] flex-none fill-kumo-warning text-kumo-warning"
/>
<p className="leading-snug">{message}</p>
</div>
</div>
<div className="space-y-2">
<p className="text-sm font-medium text-kumo-subtle">After</p>
<div className="flex max-w-72 items-start gap-2 rounded-md bg-kumo-warning-tint px-3 py-2 text-sm text-kumo-warning">
<span className="flex h-[1lh] flex-none items-center fill-kumo-warning leading-snug">
<Warning
weight="fill"
className="size-[1em] flex-none fill-kumo-warning text-kumo-warning"
/>
</span>
<p className="leading-snug">{message}</p>
</div>
</div>
</div>
);
}

/** Banner with custom React content in description. */
export function BannerCustomContentDemo() {
return (
Expand Down
25 changes: 25 additions & 0 deletions packages/kumo-docs-astro/src/pages/components/banner.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ import {
BannerAlertDemo,
BannerErrorDemo,
BannerSecondaryDemo,
BannerTitleOnlyDemo,
BannerWithIconDemo,
BannerIconAlignmentPreview,
BannerWithActionDemo,
BannerWithActionsDemo,
BannerCompactDemo,
Expand Down Expand Up @@ -111,6 +113,29 @@ export default function Example() {
<BannerWithIconDemo client:visible />
</ComponentExample>

#### Title only

<ComponentExample demo="BannerTitleOnlyDemo">
<BannerTitleOnlyDemo client:visible />
</ComponentExample>

#### Icon alignment with wrapping text

Icons align to the first line of banner text and keep their size when the message wraps.

<ComponentExample
code={`<Banner
size="sm"
icon={<Warning weight="fill" />}
variant="alert"
description="A DNS record already exists in this zone and must be reviewed before continuing."
/>`}
vrSection="banner-icon-alignment"
vrTitle="Banner Icon Alignment"
>
<BannerIconAlignmentPreview client:visible />
</ComponentExample>

### With action

<ComponentExample demo="BannerWithActionDemo">
Expand Down
23 changes: 20 additions & 3 deletions packages/kumo/src/components/banner/banner.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,28 @@ describe("Banner", () => {
expect(className).toContain("px-3");
expect(className).toContain("py-2");
expect(className).toContain("text-sm");
// Compact banners align everything on one centered row.
expect(className).toContain("items-center");
// Compact banners align icons to the first text line when content wraps.
expect(className).toContain("items-start");
// Base-size spacing/alignment must not leak in.
expect(className).not.toContain("px-4");
expect(className).not.toContain("items-start");
});

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");
Comment on lines +148 to +163

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: we shouldn't test on the existence of class names, it's brittle

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

truth! I will fix this

});

it("defaults Banner.Action children to xs in an sm banner", () => {
Expand Down
55 changes: 33 additions & 22 deletions packages/kumo/src/components/banner/banner.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {
type HTMLAttributes,
type ReactElement,
type ReactNode,
cloneElement,
forwardRef,
isValidElement,
} from "react";
Expand Down Expand Up @@ -46,7 +48,7 @@ export const KUMO_BANNER_VARIANTS = {
description: "Default banner size",
},
sm: {
classes: "items-center gap-2 rounded-md px-3 py-2 text-sm",
classes: "items-start gap-2 rounded-md px-3 py-2 text-sm",
description: "Compact banner for dialogs and tight spaces",
},
},
Expand All @@ -63,28 +65,51 @@ export type KumoBannerSize = keyof typeof KUMO_BANNER_VARIANTS.size;

/**
* Per-size render-site classes not carried by `bannerVariants` (which only emits
* the container classes). `row` is the title↔action flex gap, `icon` the icon
* wrapper height, `description` the description text size, and `action` the size
* the container classes). `row` is the title↔action flex gap,
* `description` the description text size, and `action` the size
* that child `Banner.Action`s inherit via {@link BannerActionContext}.
*/
const BANNER_SIZE_PARTS: Record<
KumoBannerSize,
{ row: string; icon: string; description: string; action: BannerActionSize }
{ row: string; description: string; action: BannerActionSize }
> = {
base: {
row: "gap-3",
icon: "h-[1.375em]",
description: "text-sm",
action: "sm",
},
sm: {
row: "gap-2",
icon: "h-[1.25em]",
description: "text-sm",
action: "xs",
},
};

const renderBannerIcon = (icon: ReactNode, className?: string) => {
const iconElement = isValidElement(icon)
? (icon as ReactElement<{ className?: string }>)
: null;
const alignedIcon = iconElement
? cloneElement(iconElement, {
className: cn(
iconElement.props.className,
"size-[1em] flex-none leading-snug",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Intentional. Custom sizes downstream is a defect (in my opinion), but point heard about the impact.

),
})
: icon;

return (
<span
className={cn(
"flex h-[1lh] flex-none items-center leading-snug",
className,
)}
>
{alignedIcon}
</span>
);
};

// The `Banner.Action` CTA compound lives in ./banner-action
// and is attached to `Banner` via Object.assign at the bottom of this file.
export type {
Expand Down Expand Up @@ -253,17 +278,7 @@ const BannerRoot = forwardRef<HTMLDivElement, BannerProps>(function BannerRoot(
className={cn(bannerVariants({ variant, size }), className)}
{...props}
>
{icon && (
<span
className={cn(
"flex shrink-0 items-center",
sizeParts.icon,
variantConfig.iconClasses,
)}
>
{icon}
</span>
)}
{icon && renderBannerIcon(icon, variantConfig.iconClasses)}
<div
className={cn(
"flex min-w-0 flex-1 items-center justify-between",
Expand Down Expand Up @@ -328,11 +343,7 @@ const BannerRoot = forwardRef<HTMLDivElement, BannerProps>(function BannerRoot(
className={cn(bannerVariants({ variant, size }), className)}
{...props}
>
{icon && (
<span className={cn("shrink-0", variantConfig.iconClasses)}>
{icon}
</span>
)}
{icon && renderBannerIcon(icon, variantConfig.iconClasses)}
{content}
</div>
</BannerActionContext.Provider>
Expand Down
4 changes: 3 additions & 1 deletion packages/kumo/src/components/toast/toast.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,8 @@ function ToastIcon({ variant }: { variant?: KumoToastVariant }) {
if (!("icon" in variantConfig)) return null;
const Icon = variantConfig.icon;
return (
<Icon data-toast-icon className="mt-0.5 h-4 w-4 shrink-0" weight="fill" />
<span className="flex h-[1lh] flex-none items-center leading-5">
<Icon data-toast-icon className="size-[1em] flex-none" weight="fill" />
</span>
);
}
Loading