-
Notifications
You must be signed in to change notification settings - Fork 322
feat(ui): make project cards interactive #509
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
|
@DEVANSH-GAJJAR is attempting to deploy a commit to the AOSSIE Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughProject cards on the projects page are now wrapped with Next.js Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Nitpick comments (1)
src/pages/projects.jsx (1)
38-39: Modernize the Link usage for Next.js 14.1.3.The
legacyBehaviorwithpassHrefpattern is outdated. Next.js 14.1.3 supports modern Link syntax without requiring an intermediate<a>element.Consider this refactor:
- <Link href={projectUrl} passHref legacyBehavior> - <a target="_blank" rel="noopener noreferrer" style={{ textDecoration: 'none', display: 'block', height: '100%' }}> + <Link + href={projectUrl} + target={projectUrl !== '#' ? "_blank" : undefined} + rel={projectUrl !== '#' ? "noopener noreferrer" : undefined} + style={{ textDecoration: 'none', display: 'block', height: '100%' }} + > <MuiCard ... </MuiCard> - </a> </Link>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/projects.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/projects.jsx (1)
src/helper/projects.js (1)
projects(12-85)
🔇 Additional comments (2)
src/pages/projects.jsx (2)
13-14: LGTM!The imports are correctly added to support the new interactive card functionality.
98-103: LGTM!The link label display and hover effects are implemented correctly with proper use of the extracted
linkLabeland group hover classes.
| <Image | ||
| src={project.logo} | ||
| alt={`${project.title} image`} | ||
| width={80} | ||
| height={80} | ||
| style={{ margin: '0 auto 16px', objectFit: 'contain' }} | ||
| /> |
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.
Fix incorrect property in alt text.
Line 64 uses project.title for the alt text, but the project data structure uses project.name. This will result in alt text like "undefined image", breaking accessibility for screen readers.
🔎 Proposed fix
<Image
src={project.logo}
- alt={`${project.title} image`}
+ alt={`${project.name} logo`}
width={80}
height={80}
style={{ margin: '0 auto 16px', objectFit: 'contain' }}
/>Note: Changed "image" to "logo" for more accurate description.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Image | |
| src={project.logo} | |
| alt={`${project.title} image`} | |
| width={80} | |
| height={80} | |
| style={{ margin: '0 auto 16px', objectFit: 'contain' }} | |
| /> | |
| <Image | |
| src={project.logo} | |
| alt={`${project.name} logo`} | |
| width={80} | |
| height={80} | |
| style={{ margin: '0 auto 16px', objectFit: 'contain' }} | |
| /> |
🤖 Prompt for AI Agents
In src/pages/projects.jsx around lines 62 to 68, the Image alt prop uses
project.title which doesn't exist on the project object; replace it with
project.name and update the description from "image" to "logo" so the alt reads
`${project.name} logo`, ensuring correct accessibility text.
Summary
Enhanced the user experience on the Projects page by making project cards fully interactive and aligning them with the AOSSIE brand identity.
Changes
Linkcomponent so the entire card is clickable.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.