-
Notifications
You must be signed in to change notification settings - Fork 322
Improve card hover effect with smoother animation #527
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
|
Someone is attempting to deploy a commit to the AOSSIE Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughTwo components receive updates: CardEffect undergoes a comprehensive visual redesign with a multi-layer card structure featuring front and back sides that transition on hover with a glow effect, while Footer updates an accessibility label for a social link. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 2
Fix all issues with AI Agents 🤖
In @src/components/CardEffect.jsx:
- Around line 5-11: The CardEffect.jsx component uses an <a> element without an
href which breaks accessibility; either make it a real link by adding an href
prop to the component (accept href in the CardEffect props, pass it into the <a>
element and use a sensible default or require it) or change the element to a
non-link semantic element (replace the opening and closing <a> in CardEffect.jsx
with a <div> or <button> and adjust any click/key handlers accordingly) so it
becomes valid HTML and keyboard-navigable.
In @src/components/Footer.jsx:
- Around line 48-49: The aria-label on the Link component is inconsistent with
the href and icon: update the Label/metadata for consistency by either (A)
revert the aria-label value back to "Follow on Twitter" so it matches the
href='https://twitter.com/aossie_org' and the faTwitter icon used in the
Link/FontAwesomeIcon elements, or (B) keep "Follow on X" but add an inline
comment next to the Link (near the aria-label/href/faTwitter usage) noting that
the service rebranded to "X" while the domain remains twitter.com and the
faTwitter icon is intentionally kept; apply the chosen change inside the Link
element where aria-label, href and faTwitter are defined.
🧹 Nitpick comments (1)
src/components/CardEffect.jsx (1)
43-50: Consider removingunoptimizedprop for better performance.The
unoptimizedprop bypasses Next.js image optimization, which can impact page load performance. Unless there's a specific reason (e.g., SVG images or external URLs that don't support optimization), consider removing this prop to benefit from automatic image optimization.🔎 Suggested change
<Image src={logo} width={140} height={140} - unoptimized className="mx-auto" alt="Project Logo" />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/CardEffect.jsxsrc/components/Footer.jsx
🔇 Additional comments (3)
src/components/CardEffect.jsx (3)
13-22: LGTM! Proper use ofaria-hiddenfor decorative glow effect.The soft glow layer is correctly marked as
aria-hidden="true"since it's purely decorative, and the transition timing coordinates well with the card's hover animations.
25-34: LGTM! Well-structured card container with smooth transitions.The card container properly implements glassmorphism with backdrop blur, responsive dark mode support, and coordinated shadow transitions. The 500ms duration with custom cubic-bezier easing creates a polished hover effect.
57-68: LGTM! Back content transitions coordinate smoothly with front content.The back content's fade-in and translate animation properly coordinates with the front content's fade-out, creating a smooth card-flip effect. The timing and easing match across both elements for a polished result.
| <a | ||
| className=" | ||
| group relative block h-[22rem] w-72 cursor-pointer | ||
| transition-transform duration-1000 ease-[cubic-bezier(0.22,1,0.36,1)] | ||
| hover:-translate-y-2 | ||
| " | ||
| > |
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.
Critical: <a> tag missing href attribute breaks accessibility.
The wrapper uses an <a> element but provides no href attribute. This violates semantic HTML and WCAG guidelines:
- The element won't be keyboard-navigable (not in tab order)
- Screen readers won't announce it properly as a link
- It's technically invalid HTML
If this card should link somewhere, add an href prop to the component. Otherwise, replace <a> with a <div> or <button>.
🔎 Proposed fix if no link is needed
-export function CardEffect({ heading, content, logo }) {
+export function CardEffect({ heading, content, logo, href }) {
return (
<a
+ href={href}
className="Or if this should not be a link:
- <a
+ <div
className="
group relative block h-[22rem] w-72 cursor-pointer
transition-transform duration-1000 ease-[cubic-bezier(0.22,1,0.36,1)]
hover:-translate-y-2
"
>And update the closing tag accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/components/CardEffect.jsx around lines 5-11, The CardEffect.jsx
component uses an <a> element without an href which breaks accessibility; either
make it a real link by adding an href prop to the component (accept href in the
CardEffect props, pass it into the <a> element and use a sensible default or
require it) or change the element to a non-link semantic element (replace the
opening and closing <a> in CardEffect.jsx with a <div> or <button> and adjust
any click/key handlers accordingly) so it becomes valid HTML and
keyboard-navigable.
| <Link aria-label="Follow on X" className=' text-zinc-400 hover:text-[#00843D] dark:text-zinc-400 dark:hover:text-yellow-400 transition' href='https://twitter.com/aossie_org'> | ||
| <FontAwesomeIcon icon={faTwitter} size='xl' /> |
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.
Consider full consistency between label, URL, and icon.
The aria-label now says "Follow on X" but the URL still points to twitter.com and uses the faTwitter icon. While the label update reflects Twitter's rebrand, the mismatch may cause minor confusion.
Consider either:
- Reverting to "Follow on Twitter" for full consistency, or
- Adding a comment explaining that Twitter/X still uses twitter.com as its domain
🤖 Prompt for AI Agents
In @src/components/Footer.jsx around lines 48-49, The aria-label on the Link
component is inconsistent with the href and icon: update the Label/metadata for
consistency by either (A) revert the aria-label value back to "Follow on
Twitter" so it matches the href='https://twitter.com/aossie_org' and the
faTwitter icon used in the Link/FontAwesomeIcon elements, or (B) keep "Follow on
X" but add an inline comment next to the Link (near the
aria-label/href/faTwitter usage) noting that the service rebranded to "X" while
the domain remains twitter.com and the faTwitter icon is intentionally kept;
apply the chosen change inside the Link element where aria-label, href and
faTwitter are defined.
What does this PR do?
Changes made
Screenshots (if applicable)
Related issue
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.