-
Notifications
You must be signed in to change notification settings - Fork 0
Miscellaneous UI Polishing #4
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
Conversation
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.
Pull Request Overview
This PR enhances the UI/UX of the lobby and leaderboard components with comprehensive styling improvements and adds equipped title badge display functionality to leaderboards. The changes focus on visual polish with gradients, animations, and improved spacing throughout the lobby interface.
- Extensive visual enhancements to lobby components with gradients, shadows, and hover effects
- New equipped title badge display on leaderboard entries for authenticated users
- Improved responsive styling and component layouts across mobile and desktop views
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/pages/Lobby.css | Comprehensive styling overhaul with gradients, transitions, hover effects, and new read-only settings display |
| client/src/components/TestConfigurator.css | Updated subject filter transitions with cubic-bezier easing and adjusted dimensions |
| client/src/components/ProfileModal.css | Added no-matches message styling for empty states |
| client/src/components/Leaderboard.jsx | Added title fetching logic and display of equipped titles on leaderboard entries |
| client/src/components/Leaderboard.css | New styling for title badges and updated leaderboard player layout structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
| } | ||
| }); | ||
| }, [leaderboard, user, authenticated, leaderboardTitlesMap]); |
Copilot
AI
Oct 31, 2025
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 useEffect has leaderboardTitlesMap in its dependency array, but the effect also updates leaderboardTitlesMap. This creates an infinite re-render loop. Remove leaderboardTitlesMap from the dependencies array since the effect checks !(netid in leaderboardTitlesMap) which will see the updated state correctly without it being a dependency.
| }, [leaderboard, user, authenticated, leaderboardTitlesMap]); | |
| }, [leaderboard, user, authenticated]); |
| ) : ( | ||
| <p className="no-results">No results found for this leaderboard.</p> | ||
| )} | ||
| {leaderboard.length > 0 ? ( leaderboard.map((entry, index) => ( <div key={`${entry.user_id}-${entry.created_at}`} className={`leaderboard-item ${user && entry.netid === user.netid ? 'current-user' : ''}`}> <span className="leaderboard-rank">{index + 1}</span> <div className="leaderboard-player"> <div className="leaderboard-avatar" onClick={() => handleAvatarClick(entry.avatar_url, entry.netid)} title={`View ${entry.netid}\'s avatar`}> <img src={entry.avatar_url || defaultProfileImage} alt={`${entry.netid} avatar`} onError={(e) => { e.target.onerror = null; e.target.src=defaultProfileImage; }} /> </div> <div className="leaderboard-player-text"> <span className="leaderboard-netid">{entry.netid}</span> {authenticated && (() => { const titles = leaderboardTitlesMap[entry.netid]; const titleToShow = titles?.find(t => t.is_equipped); return titleToShow ? ( <div className="leaderboard-titles"> <span className="leaderboard-title-badge">{titleToShow.name}</span> </div> ) : null; })()} </div> </div> <div className="leaderboard-stats"> <span className="leaderboard-wpm">{parseFloat(entry.adjusted_wpm).toFixed(0)} WPM</span> <span className="leaderboard-accuracy">{parseFloat(entry.accuracy).toFixed(1)}%</span> <span className="leaderboard-date">{period === 'daily' ? formatRelativeTime(entry.created_at) : new Date(entry.created_at).toLocaleDateString()}</span> </div> </div> )) ) : ( <p className="no-results">No results found for this leaderboard.</p> )} |
Copilot
AI
Oct 31, 2025
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.
This entire leaderboard rendering logic is condensed into a single line, making it extremely difficult to read and maintain. The JSX should be properly formatted across multiple lines with appropriate indentation.
| {leaderboard.length > 0 ? ( leaderboard.map((entry, index) => ( <div key={`${entry.user_id}-${entry.created_at}`} className={`leaderboard-item ${user && entry.netid === user.netid ? 'current-user' : ''}`}> <span className="leaderboard-rank">{index + 1}</span> <div className="leaderboard-player"> <div className="leaderboard-avatar" onClick={() => handleAvatarClick(entry.avatar_url, entry.netid)} title={`View ${entry.netid}\'s avatar`}> <img src={entry.avatar_url || defaultProfileImage} alt={`${entry.netid} avatar`} onError={(e) => { e.target.onerror = null; e.target.src=defaultProfileImage; }} /> </div> <div className="leaderboard-player-text"> <span className="leaderboard-netid">{entry.netid}</span> {authenticated && (() => { const titles = leaderboardTitlesMap[entry.netid]; const titleToShow = titles?.find(t => t.is_equipped); return titleToShow ? ( <div className="leaderboard-titles"> <span className="leaderboard-title-badge">{titleToShow.name}</span> </div> ) : null; })()} </div> </div> <div className="leaderboard-stats"> <span className="leaderboard-wpm">{parseFloat(entry.adjusted_wpm).toFixed(0)} WPM</span> <span className="leaderboard-accuracy">{parseFloat(entry.accuracy).toFixed(1)}%</span> <span className="leaderboard-date">{period === 'daily' ? formatRelativeTime(entry.created_at) : new Date(entry.created_at).toLocaleDateString()}</span> </div> </div> )) ) : ( <p className="no-results">No results found for this leaderboard.</p> )} | |
| {leaderboard.length > 0 ? ( | |
| leaderboard.map((entry, index) => ( | |
| <div | |
| key={`${entry.user_id}-${entry.created_at}`} | |
| className={`leaderboard-item ${user && entry.netid === user.netid ? 'current-user' : ''}`} | |
| > | |
| <span className="leaderboard-rank">{index + 1}</span> | |
| <div className="leaderboard-player"> | |
| <div | |
| className="leaderboard-avatar" | |
| onClick={() => handleAvatarClick(entry.avatar_url, entry.netid)} | |
| title={`View ${entry.netid}'s avatar`} | |
| > | |
| <img | |
| src={entry.avatar_url || defaultProfileImage} | |
| alt={`${entry.netid} avatar`} | |
| onError={(e) => { | |
| e.target.onerror = null; | |
| e.target.src = defaultProfileImage; | |
| }} | |
| /> | |
| </div> | |
| <div className="leaderboard-player-text"> | |
| <span className="leaderboard-netid">{entry.netid}</span> | |
| {authenticated && (() => { | |
| const titles = leaderboardTitlesMap[entry.netid]; | |
| const titleToShow = titles?.find(t => t.is_equipped); | |
| return titleToShow ? ( | |
| <div className="leaderboard-titles"> | |
| <span className="leaderboard-title-badge">{titleToShow.name}</span> | |
| </div> | |
| ) : null; | |
| })()} | |
| </div> | |
| </div> | |
| <div className="leaderboard-stats"> | |
| <span className="leaderboard-wpm"> | |
| {parseFloat(entry.adjusted_wpm).toFixed(0)} WPM | |
| </span> | |
| <span className="leaderboard-accuracy"> | |
| {parseFloat(entry.accuracy).toFixed(1)}% | |
| </span> | |
| <span className="leaderboard-date"> | |
| {period === 'daily' | |
| ? formatRelativeTime(entry.created_at) | |
| : new Date(entry.created_at).toLocaleDateString()} | |
| </span> | |
| </div> | |
| </div> | |
| )) | |
| ) : ( | |
| <p className="no-results">No results found for this leaderboard.</p> | |
| )} |
| <div | ||
| className={`leaderboard-avatar ${!authenticated ? 'disabled' : ''}`} | ||
| onClick={authenticated ? () => handleAvatarClick(entry.avatar_url, entry.netid) : undefined} | ||
| title={authenticated ? `View ${entry.netid}\'s profile` : 'Log in to view profiles'} |
Copilot
AI
Oct 31, 2025
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.
Corrected escaped apostrophe from \'s to 's (use proper quote character instead of escape sequence).
| title={authenticated ? `View ${entry.netid}\'s profile` : 'Log in to view profiles'} | |
| title={authenticated ? `View ${entry.netid}'s profile` : 'Log in to view profiles'} |
| onClick={authenticated ? () => handleAvatarClick(entry.avatar_url, entry.netid) : undefined} | ||
| title={authenticated ? `View ${entry.netid}\'s profile` : 'Log in to view profiles'} | ||
| > |
Copilot
AI
Oct 31, 2025
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 avatar element is clickable but lacks proper accessibility attributes. Add role=\"button\", tabIndex={authenticated ? 0 : -1}, and keyboard event handlers for Enter/Space keys to ensure keyboard navigation support, similar to the code that was removed from the previous implementation.
| onClick={authenticated ? () => handleAvatarClick(entry.avatar_url, entry.netid) : undefined} | |
| title={authenticated ? `View ${entry.netid}\'s profile` : 'Log in to view profiles'} | |
| > | |
| role="button" | |
| tabIndex={authenticated ? 0 : -1} | |
| onClick={authenticated ? () => handleAvatarClick(entry.avatar_url, entry.netid) : undefined} | |
| onKeyDown={authenticated ? (e => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| handleAvatarClick(entry.avatar_url, entry.netid); | |
| } | |
| }) : undefined} | |
| title={authenticated ? `View ${entry.netid}\'s profile` : 'Log in to view profiles'} |
Just some frontend updates: