refactor(ui): standardize scattered banner patterns into shared Banner component#2038
Open
evanjacobson wants to merge 18 commits intomainfrom
Open
refactor(ui): standardize scattered banner patterns into shared Banner component#2038evanjacobson wants to merge 18 commits intomainfrom
evanjacobson wants to merge 18 commits intomainfrom
Conversation
… 3 named banner files - Add green color variant to Banner - Add Banner.Dismiss subcomponent (X button) - Migrate OldSessionBanner, AdminViewingBanner, ErrorBanner to shared Banner - Save standardize-banners migration plan to .plans/
Migrate BillingBanner, FreeTrialWarningBanner, and InsufficientBalanceBanner to the shared Banner component. Add banner-showcase/ with standalone before/after comparison page and per-banner app screenshots at desktop and mobile viewports.
…fix Banner.Action gap - Add sticky controls bar with width slider, collapse/expand all buttons - Restructure screenshot galleries to 2 rows (desktop, mobile) - Make page full-width (remove max-w constraints) - Fix Banner.Action missing flex gap-2 for multi-button layouts - Re-capture screenshots with updated button spacing
Remove the banner-showcase/ standalone app (couldn't resolve parent transitive deps cleanly). Add before/after Storybook stories for all 6 Phase 1 banners under storybook/stories/banner-migration/. These use the real imported components and run in the existing Storybook infra. Also add storybook/stories/_pr/ to .gitignore for future ephemeral PR-scoped stories.
Lightweight standalone preview (~250KB) importing real components from the parent app via Vite resolve.alias. Features sidebar tabs per component, variant subtabs, and iframe-based mobile viewport toggle that correctly triggers Tailwind sm: breakpoints. Deployed to https://banner-showcase.pages.dev
BannerAction now uses flex-col sm:flex-row so multiple buttons stack on narrow viewports instead of overflowing side-by-side.
Banner.Dismiss now uses absolute positioning (top-3 right-3) so it always appears in the top-right corner regardless of flex-wrap layout or viewport size. BannerRoot gains 'relative' to establish the positioning context.
The Dismiss action button already handles dismissal — the X was calling the same onDismiss callback. Removed to avoid duplicate affordances.
BannerButton now accepts variant='primary' (default, solid fill) or variant='outline' (transparent with border). ErrorBanner uses outline for Dismiss to visually differentiate it from the primary Retry action.
The previous outline style used border-current/30 which isn't valid Tailwind v4 syntax. Added outlineButton to colorMap with explicit color classes (e.g. border-red-500/40, text-red-400, hover:bg-red-500/10) for each color.
- secondary: dark gray fill (bg-white/10), neutral — for primary actions that shouldn't carry the banner's accent color - ghost: transparent with subtle text — for de-emphasized actions ErrorBanner now uses secondary for Retry and ghost for Dismiss, matching the visual weight of the old Alert-based buttons.
Retry gets the outlined border style, Dismiss stays ghost/transparent.
Remove banner-showcase/, storybook/stories/banner-migration/, banner-test page, screenshots, and .playwright-mcp artifacts. Restore pnpm-workspace.yaml and .gitignore to main state.
…ze=lg Extend the shared Banner component with a size='lg' variant that supports the deeper color palette, circular icon backgrounds, and larger typography used by the organization welcome/success card headers. Migrate OrganizationWelcomeHeader, NewOrganizationWelcomeHeader, and OrganizationTopupSuccessHeader from bespoke Card+CardContent layouts to the shared Banner compound component.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
src/components/shared/Banner.tsx
Outdated
| return ( | ||
| <Button asChild className={btnClass}> | ||
| <Link href={href}>{children}</Link> | ||
| <Link href={href} target={target}> |
Contributor
There was a problem hiding this comment.
WARNING: target="_blank" links lose rel="noopener noreferrer"
Banner.Button now accepts target, but it does not forward a rel prop or set a safe default when opening a new tab. Call sites like OrganizationWelcomeHeader now open an external docs page with window.opener exposed. Please add rel support here and pass noopener noreferrer for _blank links.
Contributor
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Other Observations (not in diff)N/A Files Reviewed (10 files)
Reviewed by gpt-5.4-20260305 · 107,994 tokens |
evanjacobson
changed the title
|
|
||
| function BannerDismiss({ onDismiss, className }: { onDismiss: () => void; className?: string }) { | ||
| return ( | ||
| <button |
Contributor
There was a problem hiding this comment.
WARNING: Banner.Dismiss defaults to a submit button
Native <button> elements default to type="submit". If any Banner.Dismiss instance is rendered inside a form, clicking the dismiss icon will submit that form unexpectedly. Set an explicit type="button" here so the shared dismiss control stays side-effect free.
Suggested change
| <button | |
| <button type="button" |
Contributor
|
Due to the monorepo restructure you will need to recreate this PR on a new branch from main. Pass the prompt found at, https://github.com/Kilo-Org/cloud/blob/main/plans/monorepo-migration-prompt.md, to your coding agent while running in this branch. Please close this PR once done or if you don't plan to proceed with it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates 9 scattered banner/alert patterns across the codebase to use a single shared
Bannercompound component (Banner.Root,Banner.Icon,Banner.Content,Banner.Title,Banner.Description,Banner.Action,Banner.Button,Banner.Dismiss).What changed:
Bannercomponent withsize="lg"mode (for card-style headers),outline/secondary/ghostbutton variants, aBannerDismisssub-component, and per-size color mapsrole="alert"paritysize="lg"rel="noopener noreferrer"onBannerButtonwhentarget="_blank"is setLive before/after preview: https://standardize-banners-2.pages.dev
Verification
pnpm typecheck— passespnpm format— no changes neededpnpm format:check— passes (via pre-push hook)Visual Changes
See the deployed before/after showcase for all 9 migrated components at both desktop and mobile viewports:
https://standardize-banners-2.pages.dev
The showcase shows before (old pattern) and after (new Banner component) side-by-side for every component and variant.
Reviewer Notes
InsufficientBalanceBannercompact variant still uses some raw elements inside theBannerwrapper rather than the full compound component API — this is intentional because the compact layout doesn't fit the standard icon/content/action structureNewOrganizationWelcomeHeaderacceptsorganizationNamein its props type but doesn't use it in the template — this is a pre-existing issue, not introduced by this migrationBannerColorValuetype is locally declared inBillingBanner.tsxandFreeTrialWarningBanner.tsxrather than imported from Banner — kept local to avoid coupling the consumer to the component's internal type