-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Add featured products carousel #40
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
- Backend: Add is_featured and sales_count fields to Product model - Backend: Implement GET /api/products/featured endpoint with fallback to top-selling and newest - Backend: Add comprehensive tests for featured endpoint - Backend: Update seed script to set featured products deterministically - Frontend: Create FeaturedCarousel component with accessibility features - Frontend: Add useFeaturedProducts hook - Frontend: Replace FeaturedBanner with FeaturedCarousel on Home page - E2E: Add comprehensive carousel tests (navigation, keyboard, auto-advance, pause) Resolves #38
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.
Summary
This PR implements a featured products system with a three-tier fallback strategy (featured → top-selling → newest). Changes span database schema, backend API, frontend carousel component, and comprehensive test coverage.
Changes include:
- Database migration adding
is_featuredandsales_countcolumns with indexes - New
/api/products/featuredendpoint with intelligent product selection logic - React carousel component with accessibility features and auto-rotation
- Seeding logic updated for deterministic featured/sales data
Code Quality Observations:
The implementation is well-tested with good coverage of edge cases. The frontend carousel includes proper accessibility attributes (ARIA roles, keyboard navigation) and respects reduced motion preferences. However, there are some maintainability concerns flagged in inline comments regarding type casting patterns in the backend and code duplication in the frontend transformation logic.
| """Upgrade schema.""" | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.add_column('products', sa.Column('is_featured', sa.Boolean(), nullable=False, server_default=sa.text("0"))) | ||
| op.add_column('products', sa.Column('sales_count', sa.Integer(), nullable=False, server_default="0")) |
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.
Inconsistent server_default format: Line 24 uses sa.text("0") for is_featured, but this line uses a plain string "0" for sales_count. Use sa.text("0") for consistency and to ensure proper SQL generation across different databases.
| op.add_column('products', sa.Column('sales_count', sa.Integer(), nullable=False, server_default="0")) | |
| op.add_column('products', sa.Column('sales_count', sa.Integer(), nullable=False, server_default=sa.text("0"))) |
| chosen.extend(newest) | ||
|
|
||
| # Format results with image_url | ||
| result = [] |
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.
Manual dictionary construction bypasses the ProductRead response model, which can lead to inconsistencies and loses Pydantic validation benefits. The field structure here must be manually kept in sync with the ProductRead model. Consider using ProductRead.model_validate() or converting products to the proper response model instead of building dicts manually.
| chosen: List[Product] = [] | ||
| chosen_ids: set[int] = set() | ||
|
|
||
| # 1) Explicit featured products |
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.
Excessive type casting throughout this function (cast(ColumnElement, ...), cast(Any, ...)) suggests the code is fighting SQLModel's type system. This makes the code harder to read and may hide actual type issues. Consider refactoring to work with SQLModel's types more naturally, or investigate why these casts are needed.
| const { products: globalProducts } = useGlobalContext(); | ||
|
|
||
| // Transform featured products to ProductType | ||
| const featured: ProductType[] = useMemo(() => { |
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.
The Product-to-ProductType transformation logic here (lines 21-48) is duplicated from the similar transformation below (lines 52-79). Consider extracting this into a shared helper function to reduce duplication and ensure consistency.
The test was failing due to strict mode violation when multiple loading skeletons are present. Now properly handles multiple loading elements by checking the first one specifically.
Summary
Implements a dynamic carousel component for featured products on the homepage with fallback logic to top-selling and newest products.
Changes
Backend
is_featuredandsales_countfields to Product modelGET /api/products/featuredendpoint with intelligent fallback:Frontend
FeaturedCarouselcomponent with full accessibility support:useFeaturedProductshook for data fetchingTesting
Acceptance Criteria Met
✅ Landing page loads within 1 second
✅ Carousel displays properly on mobile and desktop
✅ Users can manually navigate carousel slides
✅ Clicking carousel items navigates to product detail pages
✅ Page is accessible (ARIA labels, keyboard navigation, screen reader support)
✅ No console errors or warnings
✅ All backend and E2E tests pass
✅ CI checks pass
Notes
Resolves #38