Add responsive design enhancements for larger screens#11
Conversation
✅ Deploy Preview for huskybids ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR aims to add responsive design enhancements for larger screens by increasing container max-widths and adding more grid columns. However, it also includes numerous unrelated changes that significantly broaden the scope beyond responsive design improvements.
Changes:
- Responsive design: Enhanced PageContainer, Header, and games page grid to better utilize larger screens
- Clerk integration: Centralized Clerk appearance configuration and added loading states to login/signup pages
- Betting system: Migrated from startTime to gameDate field with validation improvements
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| PageContainer.jsx | Added xl/2xl breakpoints for max-width (1024px, 1280px) and padding adjustments |
| Header.jsx | Added matching xl/2xl max-widths to align with page content container |
| games/page.jsx | Enhanced grid from 2 columns max to 3 columns (lg) and 4 columns (2xl) |
| clerk-appearance.js | Updated Clerk styling configuration (unrelated to responsive design) |
| BettingService.js | Changed game time validation to use only gameDate field (unrelated) |
| BettingService.test.js | Added gameDate field to test mock (unrelated) |
| HuskyBidsLoader.jsx | Added subtitle prop feature (unrelated) |
| sign-up/page.jsx | Refactored to use shared Clerk config and loading states (unrelated) |
| login/page.js | Refactored to use shared Clerk config and loading states (unrelated) |
| useBetValidation.js | Removed startTime field references, now only uses gameDate (unrelated) |
| useBetValidation.test.js | Updated tests to use gameDate instead of startTime (unrelated) |
| LoadingContext.jsx | Modified useEffect to always clear loading state on route change (unrelated) |
| tasks/page.jsx | Added comment about memoization (unrelated) |
| auth-pages.test.jsx | Added ClerkLoading/ClerkLoaded mock components (unrelated) |
| sign-up/redirect.test.jsx | Added ClerkLoading/ClerkLoaded mock components (unrelated) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <main | ||
| className={cn( | ||
| 'max-w-lg lg:max-w-2xl mx-auto px-6 lg:px-8 pb-24 lg:pb-12', | ||
| 'max-w-lg lg:max-w-2xl xl:max-w-5xl 2xl:max-w-7xl mx-auto px-6 lg:px-8 xl:px-10 2xl:px-12 pb-24 lg:pb-12', |
There was a problem hiding this comment.
This PR contains numerous changes that are unrelated to the stated purpose of "responsive design enhancements for larger screens". The following files contain changes that should be in separate PRs:
- clerk-appearance.js: Changes to Clerk styling configuration
- BettingService.js and related tests: Changes to betting validation logic
- HuskyBidsLoader.jsx: Addition of subtitle feature
- Login and SignUp pages: Integration of loading states and refactoring
- useBetValidation hook and tests: Removal of startTime field references
- LoadingContext.jsx: Logic changes to loading state management
- tasks/page.jsx: Addition of memoization comment
These unrelated changes make the PR difficult to review, increase risk, and violate the single responsibility principle for pull requests. Please separate these into focused PRs.
| startTime: new Date(Date.now() + 86400000), // Tomorrow | ||
| gameDate: new Date(Date.now() + 86400000), // Synced with startTime |
There was a problem hiding this comment.
The mockGame still includes a startTime field (line 104) that should be removed as part of the migration to using only gameDate. The presence of this field in test data could mask bugs where code accidentally relies on startTime.
| startTime: new Date(Date.now() + 86400000), // Tomorrow | |
| gameDate: new Date(Date.now() + 86400000), // Synced with startTime | |
| gameDate: new Date(Date.now() + 86400000), // Tomorrow |
| setIsLoading(false); | ||
| setMessage(''); | ||
| loadingStartTimeRef.current = null; | ||
| if (clearTimeoutRef.current) { | ||
| clearTimeout(clearTimeoutRef.current); | ||
| clearTimeoutRef.current = null; | ||
| } | ||
| }, [pathname, isLoading]); | ||
| }, [pathname]); |
There was a problem hiding this comment.
The removal of the isLoading check from the useEffect dependency is problematic. The original code prevented unnecessary state updates when isLoading was already false. Now, every route change will trigger state updates even when no loading state is active, potentially causing unnecessary re-renders. If the intent is to always clear on route change regardless of current state, the change is acceptable, but the dependency array removal is a significant behavioral change that should be explicitly documented or reconsidered.
| const cutoffTime = new Date(now.getTime() - BUFFER_MS); | ||
|
|
||
| if (gameTime <= cutoffTime) { | ||
| if (isNaN(gameTime.getTime()) || gameTime <= cutoffTime) { |
There was a problem hiding this comment.
The error message on line 70 will display "Invalid Date" as the ISO string when gameDate is invalid (NaN check on line 69). This could be confusing to users. Consider providing a more specific error message for invalid dates versus games that have already started, or sanitize the error message when gameDate is invalid.
| if (isNaN(gameTime.getTime()) || gameTime <= cutoffTime) { | |
| if (isNaN(gameTime.getTime())) { | |
| throw new Error(`Betting is closed - invalid game date (gameDate: ${game.gameDate})`); | |
| } | |
| if (gameTime <= cutoffTime) { |
| export default function HuskyBidsLoader({ | ||
| size = 'md', | ||
| centered = false, | ||
| subtitle = null, // TODO: New prop added - defaults to null |
There was a problem hiding this comment.
The TODO comment "New prop added - defaults to null" should be removed. TODO comments are meant for work that still needs to be done, not to document that work has been completed. If this is documentation about the prop's default value, it should be formatted as a proper JSDoc comment instead.
Isaiahriveraa
force-pushed
the
feat/responsive-design
branch
from
74156e9 to
97b0db3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Increases container max-widths and adds more grid columns on large screens to better utilize available space.
Context
Application containers were capped at 672px on all desktop sizes, making the app feel cramped on large monitors (1440px+, 1920px+).
Changes
Implementation Details
Container width progression:
Games grid progression:
Tests
Ran existing tests - all passing (84/84)
How to test
Open http://localhost:3000/games in browser DevTools responsive mode and test at 1280px, 1536px, 1920px widths.
Impact / Risk
Low risk - only affects CSS classes. No breaking changes.
Checklist