feat: enhance UI polish with focus states, keyboard navigation, and loading fallbacks #32
Conversation
WalkthroughProduct components receive accessibility and visual enhancements: focus-visible states on buttons, keyboard navigation (arrow keys) for quantity controls, new animated skeleton loaders for loading states, and empty state messaging. No API changes; additions are purely presentational and interactive. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QtyControl as ProductQuantityControl
participant Ref as Button Ref
User->>QtyControl: Press ArrowUp/ArrowRight
QtyControl->>QtyControl: handleKeyDown triggered
QtyControl->>QtyControl: Increment quantity
QtyControl->>Ref: Focus increment button
Ref->>User: Visual focus ring appears
User->>QtyControl: Press ArrowDown/ArrowLeft
QtyControl->>QtyControl: handleKeyDown triggered
QtyControl->>QtyControl: Decrement quantity
QtyControl->>Ref: Focus decrement button
Ref->>User: Visual focus ring appears
User->>QtyControl: Mouse click button
QtyControl->>QtyControl: handleMouseDown (blur)
QtyControl->>User: Focus ring removed
sequenceDiagram
participant App
participant ProductGrid as ProductGrid
participant Skeleton as ProductGridSkeleton
participant Card as ProductCard
App->>ProductGrid: Render with isLoading=true
ProductGrid->>Skeleton: Render skeleton grid
Skeleton->>User: Display animated pulse placeholders
Note over App: Data fetches...
App->>ProductGrid: Render with isLoading=false, products=[]
ProductGrid->>ProductGrid: No products, render empty state
ProductGrid->>User: Display empty state message
App->>ProductGrid: Render with isLoading=false, products=[...]
ProductGrid->>Card: Map products to ProductCard components
Card->>User: Display interactive product cards with focus states
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/product/ProductQuantityControl.tsx (1)
47-47: Consider using a theme-based focus ring color.The hard-coded
ring-blue-500might not align with your design system's theme colors. Consider using a CSS variable or Tailwind theme color for the focus ring.Example using a custom color:
- className="bg-button-secondary-bg hover:bg-button-secondary-bg-hover rounded-md aspect-square h-[24px] flex items-center justify-center transition-all duration-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-1 focus-visible:ring-blue-500 disabled:opacity-50 disabled:cursor-not-allowed active:scale-95" + className="bg-button-secondary-bg hover:bg-button-secondary-bg-hover rounded-md aspect-square h-[24px] flex items-center justify-center transition-all duration-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-1 focus-visible:ring-ring disabled:opacity-50 disabled:cursor-not-allowed active:scale-95"The same applies to line 66.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/product/ProductCard.tsx(4 hunks)src/components/product/ProductCardSkeleton.tsx(1 hunks)src/components/product/ProductGrid.tsx(1 hunks)src/components/product/ProductQuantityControl.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/product/ProductGrid.tsx (2)
src/components/product/ProductCard.tsx (2)
ProductCardProps(16-28)ProductCard(107-255)src/components/product/ProductCardSkeleton.tsx (1)
ProductGridSkeleton(27-35)
🔇 Additional comments (13)
src/components/product/ProductQuantityControl.tsx (4)
16-17: LGTM! Clean ref setup for focus management.The refs are properly typed and enable the keyboard navigation focus flow.
31-34: LGTM! Appropriate focus ring management.Blurring on mouse click ensures focus rings only appear for keyboard navigation, which aligns with modern accessibility best practices.
37-41: LGTM! Proper semantic grouping.The
role="group"with a descriptivearia-labelcorrectly groups the quantity controls for assistive technologies.
53-60: LGTM! Excellent accessibility implementation.The quantity display uses
aria-live="polite",role="status", and a descriptivearia-labelto ensure screen readers announce quantity changes appropriately. Thefont-mediumalso provides visual emphasis.src/components/product/ProductCard.tsx (4)
79-79: LGTM! Good focus and hover states.The transition, hover background, and focus-visible ring improve both polish and accessibility. Note: the
ring-blue-500is hard-coded here as well (see similar comment in ProductQuantityControl).
221-221: LGTM! Consistent focus styling.The focus-visible ring and transition enhance accessibility. Same note applies regarding the hard-coded
ring-blue-500color (see ProductQuantityControl comments).
244-244: LGTM! Consistent focus styling.Good consistency across all interactive elements. Same note about hard-coded focus ring color applies here as well.
167-167: Verify focus ring stacking through user testing; layout shift risk is minimal.Verification found:
Layout shift: The grid's
gap-6(24px) spacing adequately accommodates thescale-105hover effect on cards up to 260px wide. This is normal hover behavior and unlikely to cause disruptive shifts.Focus indicator stacking: Confirmed—when buttons inside the card receive focus, both the button's individual
focus-visible:ringand the card'sfocus-within:ringdisplay simultaneously, creating nested rings. This is a valid design choice but would benefit from UX testing to ensure it's not visually confusing to users.src/components/product/ProductCardSkeleton.tsx (2)
3-25: LGTM! Clean skeleton implementation.The skeleton structure appropriately mirrors the actual ProductCard layout with clear semantic sections. The
animate-pulseprovides a good loading indicator, and sizing matches the real component.
27-35: LGTM!Grid classes match ProductGrid perfectly. The skeleton implementation is clean with sensible defaults and maintains visual consistency across all responsive breakpoints.
src/components/product/ProductGrid.tsx (3)
8-9: LGTM! Sensible loading state props.The optional
isLoadingandskeletonCountprops with reasonable defaults enable flexible loading state management.
19-34: LGTM! Well-designed empty state.The empty state provides clear, helpful messaging with appropriate visual hierarchy. The dashed border and muted text colors effectively communicate the "empty" status.
41-53: LGTM! Clean loading state implementation.The conditional rendering clearly separates loading and loaded states, using the skeleton grid during loading and the actual product grid otherwise.
| <div className="grid w-full grid-cols-1 sm:grid-cols-2 md:grid-cols-3 xl:grid-cols-4 items-start gap-6"> | ||
| {products.map((product, index) => ( | ||
| <ProductCard | ||
| key={`${product.product_id}-${index}`} |
There was a problem hiding this comment.
🧩 Analysis chain
Consider removing index from the key.
The key combines product_id with index. If product_id is guaranteed to be unique (which it should be as a product identifier), the index is unnecessary and should be removed. Using index in keys can mask issues and cause subtle React reconciliation problems when the list changes.
If product_id is unique, apply this diff:
- key={`${product.product_id}-${index}`}
+ key={product.product_id}If there's a possibility of duplicate product_id values, that would be a data integrity issue that should be addressed at the source rather than worked around in the UI.
Verify that product_id is unique across all products:
🏁 Script executed:
#!/bin/bash
# Search for where products are fetched/defined to understand uniqueness guarantees
echo "=== Finding product data sources ==="
rg -n "product_id" --type ts --type tsx -g '!*.test.*' -C 2
echo -e "\n=== Looking for product type definitions ==="
ast-grep --pattern 'interface $_ {
$$$
product_id: $_
$$$
}'Length of output: 1596
---
🏁 Script executed:
```shell
#!/bin/bash
# Corrected search without tsx file type
echo "=== Finding product_id references in TypeScript files ==="
rg -n "product_id" --type ts -g '!*.test.*' -C 2 | head -50
echo -e "\n=== Viewing ProductGrid component ==="
cat src/components/product/ProductGrid.tsx
echo -e "\n=== Searching for product data fetching/API calls ==="
rg -n "getProduct\|fetchProduct\|products:" --type ts -C 3
Length of output: 2987
---
🏁 Script executed:
```shell
#!/bin/bash
# Use correct path prefix shown in rg output
echo "=== Viewing ProductGrid component with correct path ==="
cat git/src/components/product/ProductGrid.tsx
echo -e "\n=== Checking storefront-client for API response structure ==="
cat git/src/lib/server/storefront-client.ts | grep -A 20 "type.*ProductApiResponse"
echo -e "\n=== Looking for fetch/API call logic ==="
rg -n "getProduct|fetchProduct|storefront-client" git/src/app/\\[slug\\]/page.tsx -A 5
Length of output: 377
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the actual file structure
echo "=== Listing repository structure ==="
ls -la | head -20
echo -e "\n=== Finding ProductGrid.tsx ==="
fd ProductGrid
echo -e "\n=== Finding all TypeScript files in components/product ==="
fd . --path "*/components/product/*" --type f
echo -e "\n=== Showing directory structure ==="
find . -type d -name product | head -5
Length of output: 1673
---
🏁 Script executed:
```shell
#!/bin/bash
echo "=== Viewing ProductGrid component ==="
cat src/components/product/ProductGrid.tsx
echo -e "\n=== Viewing product type definitions ==="
cat src/type/product.ts
echo -e "\n=== Viewing page.tsx for data fetching ==="
cat src/app/\[slug\]/page.tsx
Length of output: 7204
Remove index from key—use product_id alone as the key.
The key should be product.product_id instead of ${product.product_id}-${index}. The product_id is the unique identifier returned by the API and designed to be unique. Including the array index is a React anti-pattern that can cause subtle reconciliation bugs when the list reorders; if items move positions, React loses track of component state because the key changes.
- key={`${product.product_id}-${index}`}
+ key={product.product_id}If product_id values were non-unique, that would indicate a data integrity problem that should be fixed at the API level, not worked around in the UI.
📝 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.
| key={`${product.product_id}-${index}`} | |
| key={product.product_id} |
🤖 Prompt for AI Agents
In src/components/product/ProductGrid.tsx around line 47, the list item key
currently concatenates the index with product.product_id; remove the index and
use product.product_id alone as the key. Edit the JSX to set
key={product.product_id} (ensure the prop exists and is a stable primitive),
then run the component to verify no TypeScript or runtime errors and that list
reordering preserves component state.
| const handleKeyDown = (e: React.KeyboardEvent, type: 'increment' | 'decrement') => { | ||
| if (e.key === "ArrowRight" || e.key === "ArrowUp") { | ||
| e.preventDefault(); | ||
| onIncrement(); | ||
| incrementRef.current?.focus(); | ||
| } else if (e.key === "ArrowLeft" || e.key === "ArrowDown") { | ||
| e.preventDefault(); | ||
| onDecrement(); | ||
| decrementRef.current?.focus(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Remove unused type parameter.
The type parameter is defined but never referenced in the function body. The keyboard navigation logic works correctly without it.
Apply this diff:
- const handleKeyDown = (e: React.KeyboardEvent, type: 'increment' | 'decrement') => {
+ const handleKeyDown = (e: React.KeyboardEvent) => {
if (e.key === "ArrowRight" || e.key === "ArrowUp") {
e.preventDefault();
onIncrement();
incrementRef.current?.focus();
} else if (e.key === "ArrowLeft" || e.key === "ArrowDown") {
e.preventDefault();
onDecrement();
decrementRef.current?.focus();
}
};Then update the onKeyDown handlers:
- onKeyDown={(e) => handleKeyDown(e, 'decrement')}
+ onKeyDown={handleKeyDown}- onKeyDown={(e) => handleKeyDown(e, 'increment')}
+ onKeyDown={handleKeyDown}Committable suggestion skipped: line range outside the PR's diff.
|
@Amanfromearth can you review - this is a good PR |
|
@ritoban23 Can you change outline/focus colors to match brand accent |
|
And rebase the pr too |
3 participants
Summary
This PR enhances the storefront UI with improved focus states, keyboard navigation, animated loading skeletons, and empty state feedback for product grids. It significantly improves accessibility and user experience.
Related issues
Closes #31
Changes
Screenshots / recordings (if UI)
How to test
Checklist
Summary by CodeRabbit
New Features
Style