feat(Alert): improve icon handling and Storybook controls#8
feat(Alert): improve icon handling and Storybook controls#8garrity-miepub wants to merge 2 commits intomainfrom
Conversation
- Use React.isValidElement() check to handle empty objects from Storybook - Change icon prop type from ReactNode to ReactElement for stricter typing - Replace inline SVG dismiss button with Lucide X icon - Convert stories from render functions to args pattern for working controls - Add Lucide icon dropdown selector in Storybook - Add semantic icons to variant stories (CheckCircle, AlertTriangle, etc.)
There was a problem hiding this comment.
Pull request overview
Improves the Alert component’s icon handling and Storybook usability by tightening icon typing, switching the dismiss glyph to a Lucide icon, and updating stories to work better with controls.
Changes:
- Add
React.isValidElement()gating for icon rendering and conditional left padding. - Replace inline dismiss SVG with
lucide-react’sXicon. - Convert Alert stories to args-based stories and add a selectable icon control via an icon registry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/components/Alert/Alert.tsx | Updates icon rendering/padding logic and swaps dismiss SVG to Lucide X. |
| src/components/Alert/Alert.stories.tsx | Adds Lucide icon selector mapping and refactors stories to args for working controls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| className={cn( | ||
| alertVariants({ variant }), | ||
| React.isValidElement(icon) && 'pl-11', | ||
| dismissible && 'pr-10', | ||
| className | ||
| )} | ||
| {...props} | ||
| > | ||
| {icon} | ||
| {React.isValidElement(icon) && icon} | ||
| <div>{children}</div> |
There was a problem hiding this comment.
icon is now treated as “present” whenever it’s a valid React element, but the CSS positioning rules only target direct svg children ([&>svg] / [&>svg+div]). Passing a non-SVG element (still allowed by React.ReactElement) will now add pl-11 and render the icon in normal flow (not absolutely positioned), causing misalignment compared to previous behavior (previously padding only applied when an actual svg existed). Consider either (a) constraining icon to an SVG element type, or (b) wrapping the icon in a dedicated element/class and updating selectors to target that wrapper instead of svg specifically.
| // Icon registry for Storybook controls | ||
| const iconRegistry: Record<string, React.ReactElement> = { | ||
| none: undefined as unknown as React.ReactElement, |
There was a problem hiding this comment.
This story file references the React namespace in a type (React.ReactElement) but only imports useState from react. Unless the project has global React types enabled (uncommon), this will fail typecheck. Import the React types (e.g., import type { ReactElement } from 'react' or import type * as React from 'react') and use that in the iconRegistry type.
| const iconRegistry: Record<string, React.ReactElement> = { | ||
| none: undefined as unknown as React.ReactElement, |
There was a problem hiding this comment.
iconRegistry uses none: undefined as unknown as React.ReactElement, which defeats the stricter icon typing and relies on a cast that can hide real mistakes. Prefer making the registry value type explicitly nullable/optional (e.g., Record<string, ReactElement | null> with none: null) so the Storybook mapping and Alert prop types align without unsafe assertions.
| const iconRegistry: Record<string, React.ReactElement> = { | |
| none: undefined as unknown as React.ReactElement, | |
| const iconRegistry: Record<string, React.ReactElement | null> = { | |
| none: null, |
| alertVariants({ variant }), | ||
| React.isValidElement(icon) && 'pl-11', | ||
| dismissible && 'pr-10', | ||
| className | ||
| )} | ||
| {...props} | ||
| > | ||
| {icon} | ||
| {React.isValidElement(icon) && icon} |
There was a problem hiding this comment.
React.isValidElement(icon) is evaluated twice in the same render path (for pl-11 and for conditional rendering). Consider storing it in a local const hasIcon = React.isValidElement(icon) to avoid repetition and keep the conditions in sync.
Deploying ui with
|
| Latest commit: |
ba54213
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0983293c.ui-6d0.pages.dev |
| Branch Preview URL: | https://feature-alerts-update.ui-6d0.pages.dev |
Addresses Copilot review comment on PR #145: Step 8.3 audits raw <table> elements but the Step 10 Compliance Summary template had no corresponding row to record that metric. Added the missing row to keep the audit commands and report template in sync. This is the only change from the Copilot review we accepted. Comments #1 (Step 10 intro wording) and #3 (rule #8 append vs checkbox) were reviewed and intentionally skipped — see PR comments for rationale.
alert-component.mov