Skip to content

Standardize Header Icon Spacing #480

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.

Sign up for GitHub

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

Merged
ngoiyaeric merged 1 commit into from
Feb 2, 2026
Merged

Standardize Header Icon Spacing #480

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions components/header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,24 +66,21 @@ export const Header = () => {
</h1>
</div>

<div className="w-1/2 gap-20 hidden md:flex justify-between px-10 items-center z-10">

<ProfileToggle/>
<div className="flex-1 hidden md:flex justify-center gap-10 items-center z-10">
<ProfileToggle/>

<MapToggle />

<Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar" data-testid="calendar-toggle">
<CalendarDays className="h-[1.2rem] w-[1.2rem]" />
</Button>

<div id="header-search-portal" />
<div id="header-search-portal" className="contents" />

Comment on lines +78 to 79
Copy link
Copy Markdown

@charliecreates charliecreates Bot Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

className="contents" makes the portal wrapper disappear from layout, which is likely what you want for flex spacing, but it can have side effects: it removes the element’s own box, which can impact things like anchoring, position: relative descendants, and some accessibility/tree expectations (and is historically spotty in some edge cases). If the portal content ever needs a stable box for sizing/positioning, this will be a hard-to-debug regression.

Consider instead rendering the portal wrapper conditionally only when it has content, or using hidden/sr-only patterns based on whether it’s empty (if you have a way to know). If you must keep display: contents, a short comment explaining why would help future maintainers avoid “fixing” it back and reintroducing spacing issues.

Suggestion

If the portal content is optional, prefer only creating a flex child when you actually mount content. For example, introduce a small PortalSlot component that returns null when unused, or pass a boolean like isSearchEnabled and do:

{isSearchEnabled ? <div id="header-search-portal" /> : null}

If you need the node to always exist for portal mounting, add a clarifying comment and consider a non-flex-affecting wrapper strategy (e.g., place the portal outside the flex row and absolutely position the injected content if needed).

Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing a safer portal-slot approach (or adding an explanatory comment if that’s the intended long-term behavior).

<Button variant="ghost" size="icon" onClick={handleUsageToggle}>
<TentTree className="h-[1.2rem] w-[1.2rem]" />
</Button>

<div id="timezone-clock-portal" />

<ModeToggle />

<HistoryContainer location="header" />
Expand Down