Feature/product card improvements#490
Conversation
WalkthroughThis PR enhances the ProductCard component with layout adjustments and adds a new story variant. Changes include conditional image rendering, adjusted tag positioning, restructured footer spacing, and modified action/link handling. A changeset entry documents the minor version bump. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (3)
51-56: Refactor magic number to use calculated positioning or CSS variables.The
top-[165px]value is tightly coupled to the image height of 180px (165 = 180 - 15). If the image height changes in the future, this absolute positioning will break. Consider using a CSS variable, a calculated value, or a more maintainable approach.♻️ Suggested approaches
Option 1: Use CSS variables for better maintainability
Define the offset at the component level:
export const ProductCard: React.FC<ProductCardProps> = ({ title, description, price, image, tags, status, link, action, LinkComponent, }) => { + const IMAGE_HEIGHT = 180; + const TAG_OFFSET = 15; + const tagTopPosition = IMAGE_HEIGHT - TAG_OFFSET; + return ( - <div className={cn('flex flex-col bg-card rounded-lg border border-border shadow-sm relative w-full h-full')}> + <div + className={cn('flex flex-col bg-card rounded-lg border border-border shadow-sm relative w-full h-full')} + style={{ '--image-height': `${IMAGE_HEIGHT}px`, '--tag-top': `${tagTopPosition}px` } as React.CSSProperties} + >Then update the tag positioning:
<ul className={cn( 'flex flex-wrap gap-2', - image?.url && image?.alt && 'absolute top-[165px] left-6', + image?.url && image?.alt && 'absolute left-6', )} + style={image?.url && image?.alt ? { top: `var(--tag-top)` } : undefined} >Option 2: Use Tailwind arbitrary value with calculation
<ul className={cn( 'flex flex-wrap gap-2', - image?.url && image?.alt && 'absolute top-[165px] left-6', + image?.url && image?.alt && 'absolute top-[calc(180px-15px)] left-6', )} >
82-86: Remove unnecessary wrapper div with empty className.The status badge is wrapped in a
<div className="">that appears to serve no purpose. Unless there's a specific layout reason for this wrapper, consider removing it to simplify the DOM structure.♻️ Proposed simplification
- {status && ( - <div className=""> - <Badge key={status.label} variant={status.variant} className="w-fit"> - {status.label} - </Badge> - </div> - )} + {status && ( + <Badge key={status.label} variant={status.variant} className="w-fit"> + {status.label} + </Badge> + )}
30-41: Consider extracting repeated condition to improve readability.The condition
image?.url && image?.altis repeated in two places (lines 30 and 54). Extracting this to a boolean variable would improve code maintainability and readability.♻️ Proposed refactor
export const ProductCard: React.FC<ProductCardProps> = ({ title, description, price, image, tags, status, link, action, LinkComponent, }) => { + const hasImage = Boolean(image?.url && image?.alt); + return ( <div className={cn('flex flex-col bg-card rounded-lg border border-border shadow-sm relative w-full h-full')}> {/* Image section */} - {image?.url && image?.alt && ( + {hasImage && ( <div className="relative overflow-hidden h-[180px] shrink-0 rounded-t-lg"> <Image src={image.url} alt={image.alt} sizes="180px" fill className="object-cover object-center" priority={image.priority} /> </div> )} <div className="p-6 flex flex-col gap-6 h-full"> {/* Content section */} <div className="flex flex-col h-full"> <div className="flex flex-col gap-4"> <Typography variant="highlightedSmall" className="line-clamp-2"> {title} </Typography> {tags && tags.length > 0 && ( <ul className={cn( 'flex flex-wrap gap-2', - image?.url && image?.alt && 'absolute top-[165px] left-6', + hasImage && 'absolute top-[165px] left-6', )} >Also applies to: 51-56
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/dry-rockets-kick.mdpackages/ui/src/components/Cards/ProductCard/ProductCard.stories.tsxpackages/ui/src/components/Cards/ProductCard/ProductCard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (3)
packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/framework/src/utils/models/price.ts (1)
Price(3-7)packages/framework/src/utils/models/badge.ts (1)
Badge(1-4)
🔇 Additional comments (4)
packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (2)
30-41: LGTM! Conditional image rendering prevents empty containers.The conditional rendering based on
image?.url && image?.altensures the image block is only rendered when valid image data exists, preventing empty containers and improving the component's layout flexibility.
75-100: Footer layout restructuring looks good.The expanded spacing (gap-6) and dedicated row container for price/status improve the layout organization. The combined conditional rendering for action/link (
(action || link) &&) correctly handles all scenarios including the new WithoutActionAndLink story case.packages/ui/src/components/Cards/ProductCard/ProductCard.stories.tsx (1)
123-129: LGTM! New story variant provides valuable test coverage.The
WithoutActionAndLinkstory complements the existingWithoutActionandWithoutLinkstories by testing the scenario where both are undefined. This aligns well with the updated conditional rendering logic in ProductCard.tsx (line 90) and ensures the footer action section doesn't render when neither action nor link is provided..changeset/dry-rockets-kick.md (1)
1-5: LGTM! Changeset properly documents the enhancement.The changeset correctly marks this as a minor version bump for
@o2s/uiand appropriately describes the layout adjustments to the ProductCard component.
| <ul | ||
| className={cn( | ||
| 'flex flex-wrap gap-2', | ||
| image?.url && image?.alt && 'absolute top-[165px] left-6', | ||
| )} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Execute the provided search for responsive handling
rg -n --type=tsx --type=ts -C3 'ProductCard.*tag' packages/ui/src/components/Cards/ProductCard/
# Look for responsive design tests or viewport testing in stories
rg -n --type=tsx --type=ts -C3 'viewport|breakpoint|responsive' packages/ui/src/components/Cards/ProductCard/Repository: o2sdev/openselfservice
Length of output: 121
🏁 Script executed:
# First, find the ProductCard file and read its full content
fd -n ProductCard packages/ui/src/components/Cards/ProductCard/ -type fRepository: o2sdev/openselfservice
Length of output: 296
🏁 Script executed:
# Check the structure of the Cards directory
find packages/ui/src/components/Cards -type f -name "*.tsx" -o -name "*.ts" | head -20Repository: o2sdev/openselfservice
Length of output: 1301
🏁 Script executed:
# Search for ProductCard files in the repository
find packages/ui/src -type f -name "*ProductCard*"Repository: o2sdev/openselfservice
Length of output: 261
🏁 Script executed:
# Also search in case it's named differently
rg -l 'ProductCard' packages/ui/src/components/Cards/ 2>/dev/null | head -10Repository: o2sdev/openselfservice
Length of output: 357
🏁 Script executed:
# Get all TypeScript/TSX files in the Cards directory
find packages/ui/src/components/Cards -type f \( -name "*.tsx" -o -name "*.ts" \)Repository: o2sdev/openselfservice
Length of output: 1593
🏁 Script executed:
# Check the line count of ProductCard.tsx
wc -l packages/ui/src/components/Cards/ProductCard/ProductCard.tsxRepository: o2sdev/openselfservice
Length of output: 130
🏁 Script executed:
# Read the ProductCard component to see the full context
cat -n packages/ui/src/components/Cards/ProductCard/ProductCard.tsxRepository: o2sdev/openselfservice
Length of output: 4883
🏁 Script executed:
# Check the stories file for responsive design examples
cat -n packages/ui/src/components/Cards/ProductCard/ProductCard.stories.tsxRepository: o2sdev/openselfservice
Length of output: 5967
Add responsive variants to absolutely positioned tag positioning.
The tag positioning at line 54 uses hard-coded pixel values (top-[165px], left-6) with no responsive breakpoint variants. On smaller screens, this fixed positioning may cause tags to overlap with text content or extend beyond card boundaries. Apply responsive utilities (e.g., sm:top-[165px]) or consider adjusting the positioning strategy for mobile viewports. Additionally, add viewport-specific test stories to verify tag placement across different screen sizes.
2 participants
What does this PR do?
Related Ticket(s)
Key Changes
How to test
Media (Loom or gif)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.