Fix responsive layout issues on tablet and mobile (#18) - #66
pragnyamehar-create wants to merge 2 commits into
Conversation
- Remove inline flex-direction:row on .welcome-card that was silently overriding the existing mobile stacking rule, so the welcome banner now actually stacks on tablet/mobile instead of staying cramped in a row. - Move welcome-badge width/height/font-size into CSS so they can be shrunk responsively instead of being locked by inline styles. - Let Student Profile info rows (college/branch/CGPA/etc.) wrap instead of clipping long values; long text now breaks onto a new line and stacks label-over-value on small phones. - Allow topic timeline cards to wrap their header (title + status badge) and force min-width:0 on flex children so long topic names, descriptions, and resource links can no longer push cards past the viewport width. - Add container/card padding, heading, and icon size reductions for tablet (<=992px), mobile (<=768px) and small phones (<=480px) so spacing isn't cramped and text stays readable. - Tighten dashboard tab bar and semester tab bar for small screens and guarantee horizontal scroll (with clipped-content safety via overflow-x: hidden on <=992px) instead of clipping. - Shrink and reposition the floating AI chat button on mobile (48px, safe-area-aware bottom offset) and add bottom padding to the dashboard page so it never overlaps the last card while scrolling.
|
@claude is attempting to deploy a commit to the arpit2006's projects Team on Vercel. A member of the Team first needs to authorize it. |
📋 Pull Request Preview📊 Summary
📂 File Breakdown
⏱️ Estimated Review Time6 minute(s) ✅ Reviewer Checklist
🤖 Generated automatically by the PR Preview Bot |
👋 Welcome Back!Thanks for contributing to CampusCompass again! We appreciate your continued support ❤️ 🌟 General Tips
Happy Coding! 🚀 Generated automatically by Contributor Learning Assistant. |
📏 Pull Request Size Report🟡 Classification: Medium
Size Meter
Recommendation👍 Reasonable size, but reviewers may take a little longer. Generated automatically by PR Size Analyzer. |
|
|
|
🎉 Thank you @pragnyamehar-create for submitting a Pull Request! We're excited to review your contribution. Before Review✅ Ensure all CI checks pass 📖 Before your PR is reviewed, check the PR Checklist in our Contributing Guide. Happy Contributing! 🚀 |
|
@arpit2006 could u pls check and review my PR |
⏳ Please Be PatientHi @pragnyamehar-create 👋 We noticed that you mentioned @arpit2006 regarding this Pull Request. Your contribution has been received and will be reviewed by the maintainers. Please be patient while the PR goes through the review and merging process. Maintainers may need some time to:
Please avoid repeatedly mentioning maintainers for merge requests. Thank you for your contribution and patience! ❤️ — 🤖 Automated Repository Bot |
arpit2006
left a comment
arpit2006
left a comment
There was a problem hiding this comment.
PR #66 — Responsive Dashboard (Tablet/Mobile) — Code Review
Branch: fix/responsive-dashboard-tablet-mobile-issue-18
Files changed: views/dashboard.ejs (+5 / -5), public/css/style.css (+221 / -2)
Root Cause Assessment ✅
The described root cause is accurate and well-explained. The base .welcome-card rule in CSS (line 1800) already declares display: flex, justify-content: space-between, and align-items: center. The old inline style then repeated display: flex; flex-direction: row; justify-content: space-between; align-items: center; — inline styles beat external stylesheets unconditionally, so the existing flex-direction: column media query rule (line 2922) was silently dead code. Removing those from the inline style is exactly the right fix.
Change-by-Change Analysis
views/dashboard.ejs
| Line | Change | Verdict |
|---|---|---|
Welcome card <section> |
Removed display: flex; flex-direction: row; justify-content: space-between; align-items: center; from inline style (kept decorative props) |
✅ Correct. These are now owned entirely by CSS (lines 1806–1808) where they can be overridden by media queries. |
Welcome card <section> |
Also removed padding: 32px from inline (PR desc doesn't call this out) |
|
Welcome badge <div> |
Removed width: 68px; height: 68px; font-size: 32px; from inline, moved to CSS |
✅ Correct. Enables the 480px shrink rule (line 4276). |
Topic desc wrapper <div> |
Added class topic-desc-wrapper |
✅ Needed for the padding: 16px !important mobile override (line 4231). |
public/css/style.css
Non-media-query global fixes (lines 4116–4161)
| Rule | Verdict |
|---|---|
overflow-wrap: break-word on .roadmap-desc-para, .topic-desc, etc. |
✅ Correct, no-viewport-width condition needed; these should always wrap gracefully. |
.info-list li { flex-wrap: wrap; row-gap: 4px; } |
✅ Correct. Doesn't break desktop since items rarely wrap there. |
.info-value { max-width: 100%; } |
✅ Correct. |
.topic-card-header { flex-wrap: wrap; row-gap: 8px; } |
✅ Correct, safe on desktop. |
.topic-card-header h4 { flex: 1 1 200px; } |
✅ Good — prevents badge from pushing title off-screen. |
min-width: 0 on .topic-item, .topic-card, .roadmap-header-section > div:first-child |
✅ Classic flex overflow fix, well-targeted. |
992px breakpoint (lines 4164–4179)
Container padding scaled to 0 20px. ✅ Correct. Note: this duplicates the html, body { overflow-x: hidden } 992px block at line 4110 — two separate @media (max-width: 992px) blocks, 54 lines apart. Not wrong, but a minor style inconsistency (could be merged).
768px breakpoint (lines 4182–4261)
.dashboard-page { padding: 24px 0 100px } — solves AI button overlap. ✅
Roadmap section / tab bar tightening: all targeted and appropriate. ✅
.dashboard-tabs scrollable without scrollbar — correct pattern. ✅
480px breakpoint (lines 4264–4298)
.welcome-badge shrink to 52×52, h1 to 21px. ✅
.info-list li { flex-direction: column !important; } on small phones. ✅
Floating AI button (lines 4303–4311)
env(safe-area-inset-bottom, 0px) fallback is correct for non-iOS. ✅
Findings
🔴 Finding #1 — padding removed from welcome-card inline, but CSS base rule doesn't match
Old inline: padding: 32px
CSS base rule (line 1804): padding: 24px 32px
Mobile override (line 4927): padding: 24px !important
The PR removed padding: 32px from the inline style. The CSS base rule already declares padding: 24px 32px (24px top/bottom, 32px left/right), so at ≥ 769px the card will now have 24px top/bottom instead of the previous 32px all-around — a visible change on desktop that isn't mentioned in the PR description or tested. Whether or not the visual difference is intentional, it should be explicitly acknowledged.
Recommendation: Either update the base CSS rule to padding: 32px to match the old inline value, or confirm the new 24px 32px behaviour is intentionally acceptable.
🟡 Finding #2 — Duplicate @media (max-width: 992px) blocks
Lines 4110–4114 and lines 4164–4179 are two separate 992px blocks in the same stylesheet, 50+ lines apart. Browsers handle this correctly (later declarations win on conflict), but it reduces maintainability.
Recommendation: Merge into one block. Low priority — request changes or note as follow-up.
🟡 Finding #3 — .topic-card-header has three definitions, one conflicting
- Line 2219: base rule —
display: flex; justify-content: space-between; align-items: center; gap: 12px; - Line 2934 (inside existing
@media (max-width: 768px)):flex-direction: column; align-items: flex-start; gap: 6px; - Line 4142 (new global rule):
flex-wrap: wrap; row-gap: 8px; - Line 4223 (new 768px rule):
padding: 14px 16px !important; - Line 4147:
h4 { flex: 1 1 200px; }
The existing 768px rule at line 2934 sets flex-direction: column + align-items: flex-start, which stacks the title and badge vertically on mobile. The new global flex-wrap: wrap (line 4142) is redundant on mobile since column direction already wraps. On desktop though, flex-wrap: wrap is a sensible addition. No actual bug here, just an overlap worth noting.
🟢 Finding #4 — !important usage is justified, but widespread
The PR uses !important on ~15 declarations inside media queries. This is necessary because the EJS template sets many of these properties as inline styles (which have higher specificity), and the PR description explicitly notes this pattern. Given the inline-style-heavy template, !important is the only way to override without touching every inline attribute. Acceptable in context.
🟢 Finding #5 — overflow-x: hidden on html, body at 992px
This is a safe "catch-all" guard. It can mask layout bugs rather than fix them, but the PR also fixes the actual overflow sources (flex min-width: 0, overflow-wrap), so using it as a fallback is reasonable.
Checklist Against PR Claims
| PR Claim | Verified? |
|---|---|
| Welcome card stacks on mobile (flex-direction fix) | ✅ |
| Welcome badge size/font moved out of inline | ✅ |
topic-desc-wrapper class hook added |
✅ |
| Container padding scales at 992/768/480px | ✅ |
| Info rows wrap instead of clipping | ✅ |
| Roadmap topic headers wrap | ✅ |
min-width: 0 on flex children |
✅ |
| Semester nav + tab bar tightened | ✅ |
overflow-x: hidden safety net |
✅ |
| Floating AI button shrinks + repositions | ✅ |
Bottom padding on .dashboard-page |
✅ |
| Desktop padding change (unannounced) |
Verdict
Request Changes on one item before approving:
Finding #1 is a blocker: the removal of
padding: 32pxfrom the inline style silently changes the desktop welcome card's top/bottom padding from 32px → 24px. This side-effect isn't mentioned in the PR description or tested. The author needs to either restorepadding: 32pxto the CSS base rule or explicitly call out the padding change as intentional.
Findings #2 and #3 are non-blocking style suggestions. Everything else is clean, well-targeted, and correctly solves the stated issues.
|
@arpit2006 Thanks for the review! Fixed Finding #1 by restoring the base .welcome-card padding to 32px, preserving the previous desktop spacing while keeping the existing 24px mobile override. I also verified the changes with git diff --check. |
|
👋 This pull request has been automatically marked as stale because it has had no activity for 14 days. It will be closed in 7 days if no further activity occurs. If you are still working on this, please:
Thank you for your contribution to CampusCompass! 🚀 |
3 participants
Here's a PR description you can paste in when opening the pull request:
Fix Responsive Layout Issues on Tablet and Mobile Devices
Fixes #18
Summary
The dashboard wasn't adapting correctly on tablet and mobile viewports — narrow container on tablets, overflowing profile text, clipped roadmap cards, and an AI chat button that covered content while scrolling. This PR addresses the root causes rather than just patching symptoms.
Root cause
Most of these bugs traced back to one thing:
.welcome-cardhad a mobile rule (flex-direction: column) already defined instyle.css, but the EJS template also setflex-direction: rowas an inline style — and inline styles always win over external stylesheet rules, regardless of media queries. So the existing "fix" was silently being overridden and never actually applied. Several other elements had the same pattern (fixed inline widths/padding/font-sizes with no responsive path).Changes
views/dashboard.ejsflex-direction: row(and other layout props now owned by CSS) from the welcome card so it can actually stack on smaller screenswidth/height/font-sizeout of inline styles and into CSS so they can shrink responsivelytopic-desc-wrapper) to the topic description container so its padding can be tuned per breakpointpublic/css/style.cssmin-width: 0added to flex children so long topic names/descriptions/resource links can't force horizontal overflowoverflow-x: hiddenon the page as a safety net against any residual horizontal scrollenv(safe-area-inset-bottom)on mobile; added bottom padding to the dashboard page so it never sits on top of the last card while scrollingTesting
dashboard.ejswith mock data viaejs.renderFile— no template errors