made header better #1676
made header better #1676
Conversation
Adjust header height, improve responsive behavior for navigation links, and update CTA button styling. Enhances visual consistency and mobile-friendliness of the header component.
📝 WalkthroughWalkthroughSingle file modification to the main layout route that adjusts header sizing, responsive visibility of navigation links, and CTA button styling. Changes include explicit header height, responsive hiding of docs and enterprise links, and consistent CTA button padding and text size increases. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/routes/_view/route.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (1)
apps/web/src/routes/_view/route.tsx (1)
173-173: LGTM! Good responsive design pattern.Hiding the Docs and Enterprise links on medium and smaller viewports while keeping them accessible in the mobile menu is a solid approach for reducing header clutter on smaller screens.
Also applies to: 191-191
| <header className="sticky top-0 bg-white/80 backdrop-blur-sm border-b border-neutral-100 z-50 h-[69px]"> | ||
| <div | ||
| className={`${maxWidthClass} mx-auto px-4 laptop:px-0 border-x border-neutral-100 py-4`} | ||
| className={`${maxWidthClass} mx-auto px-4 laptop:px-0 border-x border-neutral-100 h-full`} | ||
| > | ||
| <div className="flex items-center justify-between"> | ||
| <div className="flex items-center justify-between h-full"> |
There was a problem hiding this comment.
Fix mobile menu positioning to match new header height.
The header height has been changed to h-[69px], but the mobile menu overlay (line 265) and mobile menu (line 269) still use top-[65px]. This creates a 4px misalignment where the mobile menu overlaps with the header.
Apply this diff to fix the alignment:
- <div
- className="fixed top-[65px] left-0 right-0 bottom-0 bg-black/20 z-40 sm:hidden animate-in fade-in duration-200"
+ <div
+ className="fixed top-[69px] left-0 right-0 bottom-0 bg-black/20 z-40 sm:hidden animate-in fade-in duration-200"
onClick={() => setIsMenuOpen(false)}
/>
- <div className="fixed top-[65px] left-0 right-0 bg-white border-b border-neutral-100 shadow-lg z-50 sm:hidden animate-in slide-in-from-top duration-300 max-h-[calc(100vh-65px)] overflow-y-auto">
+ <div className="fixed top-[69px] left-0 right-0 bg-white border-b border-neutral-100 shadow-lg z-50 sm:hidden animate-in slide-in-from-top duration-300 max-h-[calc(100vh-69px)] overflow-y-auto">Note: Also update max-h-[calc(100vh-65px)] to max-h-[calc(100vh-69px)] on line 269 for correct overflow calculation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/routes/_view/route.tsx around lines 94-98 (header) and the
mobile menu code at lines ~265-269, the header height was changed to h-[69px]
but the mobile overlay and menu still use top-[65px] and
max-h-[calc(100vh-65px)]; update the mobile overlay and mobile menu top values
from top-[65px] to top-[69px], and change max-h-[calc(100vh-65px)] to
max-h-[calc(100vh-69px)] so the mobile menu aligns with the new header height
and the overflow calculation is correct.
| className="px-4 h-8 flex items-center text-sm bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | ||
| > | ||
| {platformCTA.label} | ||
| </a> | ||
| ) : ( | ||
| <button | ||
| onClick={handleCTAClick} | ||
| className="px-3 h-8 flex items-center text-xs bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | ||
| className="px-4 h-8 flex items-center text-sm bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | ||
| > | ||
| {platform === "mobile" ? "Remind me" : platformCTA.label} | ||
| {platformCTA.label} | ||
| </button> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify label consistency between mobile header and mobile menu CTAs.
The styling changes look good, but there's a potential label inconsistency:
- Line 247 (mobile header CTA): Uses
platformCTA.labeldirectly - Lines 385-387 (mobile menu CTA): Uses conditional logic
platform === "mobile" ? "Get reminder" : platformCTA.label
If the user is on a mobile platform, they might see different labels for the same action in the header versus the mobile menu.
Run the following script to check the implementation of getPlatformCTA and verify what label is returned for the mobile platform:
If platformCTA.label for mobile platform is not "Get reminder", consider standardizing the label logic by applying this diff to line 385-387:
- {platform === "mobile"
- ? "Get reminder"
- : platformCTA.label}
+ {platformCTA.label}Or, if the conditional logic in the mobile menu is correct, revert the change at line 247:
- {platformCTA.label}
+ {platform === "mobile"
+ ? "Get reminder"
+ : platformCTA.label}🏁 Script executed:
#!/bin/bash
# Description: Find the implementation of getPlatformCTA to verify mobile platform label
# Search for getPlatformCTA function definition
ast-grep --pattern 'function getPlatformCTA($$$) {
$$$
}'
# Also search for export const/function getPlatformCTA
ast-grep --pattern 'export $_ getPlatformCTA = $$$'
# Search with rg as backup
rg -n "getPlatformCTA.*=.*\(" -A 20 --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Description: Find getPlatformCTA function implementation
# Search for getPlatformCTA across all files
rg -n "getPlatformCTA" -A 30 --type=ts
# If that doesn't work, try without type specification
rg -n "getPlatformCTA" -A 30Length of output: 21669
🏁 Script executed:
#!/bin/bash
# Find lines 385-387 in route.tsx to see the mobile menu CTA
sed -n '380,390p' apps/web/src/routes/_view/route.tsx
# Also check if there's a similar pattern around those lines in index.tsx
sed -n '380,390p' apps/web/src/routes/_view/index.tsxLength of output: 896
Fix label inconsistency between mobile header and mobile menu CTAs.
Mobile users will see different labels for the same action:
- Header CTA (line 247): "Join waitlist" (from
platformCTA.label) - Mobile menu CTA (lines 385-387): "Get reminder" (from conditional logic)
Apply the conditional logic to line 247 to match the mobile menu:
- {platformCTA.label}
+ {platform === "mobile"
+ ? "Get reminder"
+ : platformCTA.label}Or standardize both to use platformCTA.label if "Get reminder" is the outdated logic.
📝 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.
| className="px-4 h-8 flex items-center text-sm bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | |
| > | |
| {platformCTA.label} | |
| </a> | |
| ) : ( | |
| <button | |
| onClick={handleCTAClick} | |
| className="px-3 h-8 flex items-center text-xs bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | |
| className="px-4 h-8 flex items-center text-sm bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | |
| > | |
| {platform === "mobile" ? "Remind me" : platformCTA.label} | |
| {platformCTA.label} | |
| </button> | |
| className="px-4 h-8 flex items-center text-sm bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | |
| > | |
| {platformCTA.label} | |
| </a> | |
| ) : ( | |
| <button | |
| onClick={handleCTAClick} | |
| className="px-4 h-8 flex items-center text-sm bg-linear-to-t from-stone-600 to-stone-500 text-white rounded-full shadow-md active:scale-[98%] transition-all" | |
| > | |
| {platform === "mobile" | |
| ? "Get reminder" | |
| : platformCTA.label} | |
| </button> |
🤖 Prompt for AI Agents
In apps/web/src/routes/_view/route.tsx around lines 238 to 248, the header CTA
uses platformCTA.label while the mobile menu CTA uses a conditional that renders
"Get reminder" in some cases; update the header anchor/button at line ~247 to
use the same conditional expression as the mobile menu so both CTAs render the
identical computed label (or alternatively standardize both places to always use
platformCTA.label if you decide "Get reminder" is outdated). Ensure you replace
the direct {platformCTA.label} with the same conditional logic used in the
mobile menu so the labels remain consistent across header and mobile menu.
Adjust header height, improve responsive behavior for navigation
links, and update CTA button styling. Enhances visual consistency
and mobile-friendliness of the header component.