added margins to certain pages to make the look consistant across the…#96
Conversation
There was a problem hiding this comment.
Pull Request Overview
Standardizes page spacing and updates UI components for a more consistent layout and improved tabs UX.
- Aligns Messages page spacing and header structure.
- Adds a sticky sidebar and adjusts tabs layout in Feed.
- Reworks Tabs component to include an animated selection indicator and updated trigger styles.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| frontend/src/pages/Messages.js | Adjusts container padding/margins and message bubble text color handling. |
| frontend/src/pages/Feed.js | Makes the sidebar sticky, tweaks header spacing, and updates TabsList layout classes. |
| frontend/src/components/ui/Tabs.js | Introduces an active tab indicator, switches to flex layout, and revises trigger styles/behavior. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| useEffect(() => { | ||
| const activeTab = tabsRef.current[currentValue]; | ||
| if (activeTab) { | ||
| setIndicatorStyle({ | ||
| left: activeTab.offsetLeft, | ||
| width: activeTab.offsetWidth, | ||
| }); | ||
| } | ||
| }, [currentValue]); |
There was a problem hiding this comment.
The active tab indicator only updates when the selected value changes. After a window resize (or container width change), the indicator can become misaligned because offsetLeft/offsetWidth change but this effect doesn't re-run. Add a resize listener or a ResizeObserver to recompute the indicator on layout changes.
| ref={buttonRef} | ||
| className={`relative flex-1 inline-flex items-center justify-center whitespace-nowrap rounded-md px-3 py-1.5 text-sm font-medium transition-colors duration-200 focus:outline-none disabled:pointer-events-none disabled:opacity-50 ${ | ||
| isActive | ||
| ? 'bg-white text-gray-900 shadow-sm' | ||
| : 'hover:bg-white hover:text-gray-900' | ||
| ? 'text-emerald-700 font-semibold' | ||
| : 'text-gray-600 hover:text-gray-900' | ||
| } ${className}`} | ||
| onClick={() => onValueChange(value)} | ||
| {...props} |
There was a problem hiding this comment.
Removing the visible focus ring makes keyboard navigation harder. Restore accessible focus-visible styles (e.g., focus-visible:ring-2 focus-visible:ring-emerald-500 focus-visible:ring-offset-2) so users can see which tab is focused.
| }, [tabsRef, value]); | ||
|
|
||
| return ( | ||
| <button |
There was a problem hiding this comment.
Add type="button" to avoid unintended form submissions if TabsTrigger is ever rendered inside a form.
| <button | |
| <button | |
| type="button" |
| return ( | ||
| <div className="container mx-auto px-4 py-6"> | ||
| <div className="mb-6"> | ||
| <div className="container mx-auto" style={{ paddingTop: '6px' }}> |
There was a problem hiding this comment.
[nitpick] Avoid inline padding styles; use Tailwind spacing utilities for consistency and theming. Suggest replacing with className="container mx-auto px-4 pt-1.5" to keep horizontal padding consistent with other pages and express 6px as pt-1.5.
| <div className="container mx-auto" style={{ paddingTop: '6px' }}> | |
| <div className="container mx-auto px-4 pt-1.5"> |
| className={`text-sm ${ | ||
| message.senderId === user.uid ? '' : 'text-gray-900' | ||
| }`} | ||
| style={message.senderId === user.uid ? { color: '#FFFFFF' } : undefined} | ||
| > | ||
| {message.text} | ||
| </p> | ||
| <p | ||
| className={`text-xs mt-1 ${ | ||
| message.senderId === user.uid ? '' : 'text-gray-500' | ||
| }`} | ||
| style={message.senderId === user.uid ? { color: '#FFFFFF' } : undefined} |
There was a problem hiding this comment.
[nitpick] Prefer Tailwind classes over inline color styles for consistency and easier theming. For example: use text-white when senderId matches and text-gray-900/text-gray-500 otherwise, e.g., className={text-sm ${message.senderId === user.uid ? 'text-white' : 'text-gray-900'}} and className={text-xs mt-1 ${message.senderId === user.uid ? 'text-white' : 'text-gray-500'}}.
| className={`text-sm ${ | |
| message.senderId === user.uid ? '' : 'text-gray-900' | |
| }`} | |
| style={message.senderId === user.uid ? { color: '#FFFFFF' } : undefined} | |
| > | |
| {message.text} | |
| </p> | |
| <p | |
| className={`text-xs mt-1 ${ | |
| message.senderId === user.uid ? '' : 'text-gray-500' | |
| }`} | |
| style={message.senderId === user.uid ? { color: '#FFFFFF' } : undefined} | |
| className={`text-sm ${message.senderId === user.uid ? 'text-white' : 'text-gray-900'}`} | |
| > | |
| {message.text} | |
| </p> | |
| <p | |
| className={`text-xs mt-1 ${message.senderId === user.uid ? 'text-white' : 'text-gray-500'}`} |
|
Minseo421
left a comment
Minseo421
left a comment
There was a problem hiding this comment.
nice work to added margins to certain pages to make the look consistant
Minseo421
left a comment
Minseo421
left a comment
There was a problem hiding this comment.
nice work to added margins to certain pages to make the look consistant
… website
📌 Description
This PR adds margins and padding to various pages so that the look is consistent across the different pages. It create a cleaner, more uniform look so the website feels more polished.
🔗 Related Issue(s)
Closes #76
✅ Changes
🧪 How to Test
Can be tested by launching the website, and checking out the various pages and noticing that the padding is quite consistant now, espcially between the header and the body on most of the pages