-
-
Notifications
You must be signed in to change notification settings - Fork 42
fix(alert): icon shrinks #84
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
fix(alert): icon shrinks #84
Conversation
WalkthroughThe Alert component’s default variant styling was updated to add a non-shrinking rule for nested SVGs by appending the class selector "[&_svg]:shrink-0" to existing classes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Kenzo-Wada is attempting to deploy a commit to the Retro UI Team on Vercel. A member of the Team first needs to authorize it. |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/retroui/Alert.tsx (1)
44-47: Type mismatch: interface declares paragraph props but component renders a div.
IAlertDescriptionPropsextends props forHTMLParagraphElementbutAlertDescriptionrenders a<div>. This can cause incorrect typing and missed attributes.Apply this diff:
-interface IAlertDescriptionProps - extends HtmlHTMLAttributes<HTMLParagraphElement> {} +interface IAlertDescriptionProps extends HTMLAttributes<HTMLDivElement> {} const AlertDescription = ({ className, ...props }: IAlertDescriptionProps) => ( <div className={cn("text-muted-foreground", className)} {...props} /> );
🧹 Nitpick comments (1)
components/retroui/Alert.tsx (1)
1-1: Use the generic React HTMLAttributes instead of HtmlHTMLAttributes.
HtmlHTMLAttributesis specific to the root<html>element. Fordiv/h5, useHTMLAttributes<T>.Apply this diff:
-import { HtmlHTMLAttributes } from "react"; +import type { HTMLAttributes } from "react"; @@ -interface IAlertProps - extends HtmlHTMLAttributes<HTMLDivElement>, +interface IAlertProps + extends HTMLAttributes<HTMLDivElement>, VariantProps<typeof alertVariants> {} @@ -interface IAlertTitleProps extends HtmlHTMLAttributes<HTMLHeadingElement> {} +interface IAlertTitleProps extends HTMLAttributes<HTMLHeadingElement> {}Also applies to: 25-27, 38-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/retroui/Alert.tsx(1 hunks)
🔇 Additional comments (1)
components/retroui/Alert.tsx (1)
10-10: Move icon shrink rule to the base cva so it applies to all variantsIcons still shrink in variant="solid"; move [&_svg]:shrink-0 into the base cva string (or duplicate it on solid).
Apply this diff:
-const alertVariants = cva("relative w-full rounded border-2 p-4", { +const alertVariants = cva("relative w-full rounded border-2 p-4 [&_svg]:shrink-0", { variants: { variant: { - default: "bg-background text-foreground [&_svg]:shrink-0", + default: "bg-background text-foreground", solid: "bg-black text-white", },Optional: narrow selector to direct children only:
-const alertVariants = cva("relative w-full rounded border-2 p-4 [&_svg]:shrink-0", { +const alertVariants = cva("relative w-full rounded border-2 p-4 [&>svg]:shrink-0", {Search for returned no matches in the repo; confirm whether Alert is used with variant="solid" before merging.
fixes: #83
Hi @ariflogs, thanks for the awesome library!
hope you'd check this bug report!
before

after

Summary by CodeRabbit
Bug Fixes
Style