Add missing home link in mobile view in navigation bar#1330
Add missing home link in mobile view in navigation bar#1330MrgSub merged 6 commits intoMail-0:stagingfrom
Conversation
|
""" WalkthroughThe mobile navigation component was updated to make the logo in the navigation sheet clickable, linking to the homepage. Additionally, a new "Home" link was added at the top of the mobile navigation menu's link list. Layout adjustments were made to support these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MobileNavSheet
participant Homepage
User->>MobileNavSheet: Open navigation menu
User->>MobileNavSheet: Tap logo or "Home" link
MobileNavSheet->>Homepage: Navigate to "/"
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
apps/mail/components/navigation.tsx
Outdated
| <div className="space-y-3 flex flex-col"> | ||
| <Link to="/" className="mt-2"> | ||
| Home | ||
| </Link> | ||
| <Link to="/pricing" className="mt-2"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the sheet closes when a menu item is tapped
On mobile, the navigation drawer (Sheet) stays open after clicking “Home”, forcing users to manually dismiss it. Invoke setOpen(false) via onClick to deliver the expected UX.
- <Link to="/" className="mt-2">
+ <Link to="/" className="mt-2" onClick={() => setOpen(false)}>
Home
</Link>Consider doing the same for the remaining internal links for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="space-y-3 flex flex-col"> | |
| <Link to="/" className="mt-2"> | |
| Home | |
| </Link> | |
| <Link to="/pricing" className="mt-2"> | |
| <div className="space-y-3 flex flex-col"> | |
| <Link to="/" className="mt-2" onClick={() => setOpen(false)}> | |
| Home | |
| </Link> | |
| <Link to="/pricing" className="mt-2"> |
🤖 Prompt for AI Agents
In apps/mail/components/navigation.tsx around lines 235 to 239, the navigation
drawer (Sheet) does not close when clicking the "Home" link, causing poor UX on
mobile. Add an onClick handler to the Link component that calls setOpen(false)
to close the drawer when the link is tapped. Apply the same onClick handler to
all other internal Link components in the navigation menu to maintain consistent
behavior.
MrgSub
left a comment
MrgSub
left a comment
There was a problem hiding this comment.
please address coderabbit
MrgSub
left a comment
MrgSub
left a comment
There was a problem hiding this comment.
Please address coderabbit
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/mail/components/navigation.tsx (1)
255-291: ❌ Missing closing</div>breaks JSX tree
<div className="flex flex-col space-y-3">opened on line 255 is never closed before</SheetContent>.
The parse error reported by Biome stems from this.@@ {resources.map((resource) => { const Icon = IconComponent[resource.platform]; return ( <Link key={resource.title} to={resource.href} className="flex items-center gap-2 font-medium" > {resource.platform && <Icon className="dark:fill-muted-foreground h-5 w-5" />} </Link> ); })} - </div> + </div> {/* closes resources grid */} + </div> {/* closes column started on line 255 */} </SheetContent>
🧹 Nitpick comments (2)
apps/mail/components/navigation.tsx (2)
255-259: Minor style consistencyAll other nav items use
font-medium; the new “Home” link does not.
Add the same class so text weight is consistent.-<Link to="/" className="mt-2" onClick={() => setOpen(false)}> +<Link to="/" className="mt-2 font-medium" onClick={() => setOpen(false)}>
158-164: UseLinkinstead of<a>for internal/pricingrouteDesktop “Pricing” button triggers a full-page reload.
Swap toLink(already imported) and close the sheet in mobile for parity.-<a href="/pricing"> +<Link to="/pricing" onClick={() => setOpen(false)}> ... -</a> +</Link>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/navigation.tsx(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/mail/components/navigation.tsx
[error] 253-253: Expected corresponding JSX closing tag for 'div'.
Opening tag
closing tag
(parse)
[error] 219-219: Expected corresponding JSX closing tag for 'SheetContent'.
Opening tag
closing tag
(parse)
🔇 Additional comments (1)
apps/mail/components/navigation.tsx (1)
281-286: External targets should open in new tab & addrelThe social links now use React Router
Linkwith absolute URLs.
Linkis intended for internal routing; for external destinations prefer<a>withtarget="_blank" rel="noopener noreferrer">to avoid SPA mis-routing.No code diff provided as this is a wider pattern – please revise or confirm intent.
2 participants
Fixes a missing home link in the mobile navbar. Previously, users could not navigate back to the homepage when using the mobile menu. This update adds the appropriate link to improve usability on smaller devices.
Resolves: #1329
Type of Change
Areas Affected
Testing Done
Verified the home link appears in the mobile view and correctly redirects to the homepage.
Security Considerations
Checklist
Screenshots/Recordings
homelink.mp4
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit