-
Notifications
You must be signed in to change notification settings - Fork 19
Fix protocol card image cropping on hover #23
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe Card component is restructured to remove the Link wrapper from the root element and render a plain div instead. The image container's overflow property changes from hidden to visible, and minor code cleanup is applied to className and description rendering syntax. Changes
Possibly related issues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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: 1
🧹 Nitpick comments (1)
components/card.tsx (1)
20-27: Consider optimizing image dimensions and improving alt text.Two minor improvements:
Image dimensions: The
widthandheightprops (700x700) significantly exceed the display size (h-64= 256px). Consider setting these closer to the actual rendered size for optimal performance.Alt text: "Protocol Logo" is generic. Consider using the
titleprop for a more descriptive alt text:alt={title}oralt={\${title} logo`}`.Apply this diff:
<Image src={image} - alt="Protocol Logo" + alt={`${title} logo`} loading="lazy" - width="700" - height="700" + width="300" + height="300" className="h-64 w-full object-contain transition duration-500 group-hover:scale-105" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/card.tsx(1 hunks)
🔇 Additional comments (1)
components/card.tsx (1)
19-28: The overflow-visible change correctly fixes image cropping on hover.The change from
overflow-hiddentooverflow-visibleallows the image to scale to 105% without being clipped. With the grid layouts usinggap-8spacing (32px), the ~13px expansion from the 5% scale is fully contained and won't cause overlap or layout issues.
| </div> | ||
| </Link> | ||
| <div | ||
| key={index} |
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.
Remove the key prop from inside the Card component.
The key prop should be applied at the parent level when rendering multiple Card components in a list, not inside the component definition itself. This placement serves no purpose and violates React conventions.
Apply this diff to remove the key prop:
- <div
- key={index}
- className="group relative flex h-auto flex-col justify-evenly rounded-lg border border-zinc-200 p-6 shadow-xl"
- >
+ <div className="group relative flex h-auto flex-col justify-evenly rounded-lg border border-zinc-200 p-6 shadow-xl">If keys are needed, apply them at the parent level:
{cards.map((card, index) => (
<Card key={index} {...card} />
))}🤖 Prompt for AI Agents
In components/card.tsx around line 16, remove the inline key prop from the Card
component JSX (delete "key={index}") because keys belong on the parent when
rendering lists; then ensure any list rendering of Card in parent components
supplies a stable key (e.g., card.id or index) when mapping over cards.
#Summary
Fixes protocol card hover behavior where images were cropped when scaled.
Change
Notes
I noticed existing PRs (#17, #19, #20) that address this issue.
This PR is submitted as part of a hackathon contribution and follows the same intended fix.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.