up #47
up #47
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a products showcase section to the homepage, featuring a new Dialog UI component and a ProductsSection component that displays products in a grid with modal details view.
Changes:
- Added reusable Dialog component based on Radix UI primitives for accessible modal interactions
- Created ProductsSection component with product cards and detailed modal views
- Integrated the new section into the home page between Projects and Team sections
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/components/ui/dialog.tsx | New Dialog component wrapper around Radix UI Dialog primitives with styled overlay, content, header, footer, title, and description components |
| src/components/home/products-section.tsx | New ProductsSection component displaying product cards in a 2-column responsive grid with Dialog-based detail views, including features list and external links |
| src/app/page.tsx | Added ProductsSection import and render between ProjectsSection and TeamMembersSection with appropriate background styling |
| const DialogHeader = ({ | ||
| className, | ||
| ...props | ||
| }: React.HTMLAttributes<HTMLDivElement>) => ( | ||
| <div | ||
| className={cn( | ||
| "flex flex-col space-y-1.5 text-center sm:text-left", | ||
| className | ||
| )} | ||
| {...props} | ||
| /> | ||
| ) | ||
| DialogHeader.displayName = "DialogHeader" |
There was a problem hiding this comment.
According to the custom coding guidelines, components should export their prop types. The DialogHeader component should export its props interface for consistency with other components in the codebase (see button.tsx, avatar.tsx, container.tsx patterns). Consider creating and exporting a DialogHeaderProps interface.
| const DialogFooter = ({ | ||
| className, | ||
| ...props | ||
| }: React.HTMLAttributes<HTMLDivElement>) => ( | ||
| <div | ||
| className={cn( | ||
| "flex flex-col-reverse sm:flex-row sm:justify-end sm:space-x-2", | ||
| className | ||
| )} | ||
| {...props} | ||
| /> | ||
| ) | ||
| DialogFooter.displayName = "DialogFooter" |
There was a problem hiding this comment.
According to the custom coding guidelines, components should export their prop types. The DialogFooter component should export its props interface for consistency with other components in the codebase (see button.tsx, avatar.tsx, container.tsx patterns). Consider creating and exporting a DialogFooterProps interface.
| } | ||
|
|
||
| export default function ProductsSection({ className }: ProductsSectionProps) { | ||
| const [selectedProduct, setSelectedProduct] = useState<Product | null>(null) |
There was a problem hiding this comment.
The selectedProduct state is set on line 152 when clicking "View Details", but it's never actually used. The Dialog component already receives the product data directly through closure scope within the map function, making this state redundant. Consider removing this unused state to simplify the component.
| variant="outline" | ||
| size="sm" | ||
| className="flex-1" | ||
| onClick={() => setSelectedProduct(product)} |
There was a problem hiding this comment.
The onClick handler sets selectedProduct but this state is never used. The Dialog already has access to the product through closure scope from the map function. Consider removing this onClick handler as it serves no purpose.
| onClick={() => setSelectedProduct(product)} |
| width={1200} | ||
| height={675} | ||
| className="w-full h-auto object-cover" | ||
| priority |
There was a problem hiding this comment.
The priority prop should be used for images that are visible above the fold on initial page load. Since this image is inside a modal dialog that only opens on user interaction, it doesn't meet the criteria for priority loading. Setting priority=true on non-critical images can negatively impact Core Web Vitals by delaying more important resources. Remove the priority prop to optimize performance.
| priority |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
No description provided.