Skip to content

fix(appshell): remove empty header and add floating menu button#1165

Merged
steilerDev merged 3 commits into
betafrom
fix/1161-remove-empty-header
Mar 22, 2026
Merged

fix(appshell): remove empty header and add floating menu button#1165
steilerDev merged 3 commits into
betafrom
fix/1161-remove-empty-header

Conversation

@steilerDev
Copy link
Copy Markdown
Owner

Summary

  • Removes the Header component that rendered an empty 60px bar on every page
  • Replaces the mobile sidebar toggle with a floating action button (FAB) visible only on mobile/tablet viewports
  • Updates AppShell tests to target the new FAB button via data-testid="menu-fab"

Fixes #1161

Test plan

  • Unit tests pass (AppShell.test.tsx updated)
  • Desktop: no visible toggle button, sidebar always visible
  • Mobile/tablet: floating FAB in bottom-right opens/closes sidebar
  • FAB has proper focus-visible styling and touch targets
  • Dark mode: FAB colors adapt correctly

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

Copy link
Copy Markdown
Owner Author

@steilerDev steilerDev left a comment

Choose a reason for hiding this comment

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

[product-architect]

Clean, well-scoped refactor. Architecture compliance verified:

  • Component removal: Header component fully removed (TSX, CSS module, tests). No orphaned imports remain.
  • Design tokens: FAB styles correctly use design tokens throughout (--spacing-*, --color-*, --radius-full, --shadow-*, --transition-button, --z-overlay). No hardcoded values.
  • Accessibility: Proper aria-label toggling, type="button", minimum 44px touch target, focus-visible styling, prefers-reduced-motion support.
  • i18n: Uses useTranslation('common') with existing aria.openMenu / aria.closeMenu keys -- consistent with the deleted Header's approach.
  • Test coverage: Tests updated to match the new FAB pattern. One new test case added for attribute verification. The within(header) scoping correctly replaced with direct getByTestId.

One informational note: the deleted Header.module.css had a few hardcoded values (1rem, 0.25rem, 1.5rem, 0.2s ease) whereas the new FAB styles consistently use design tokens -- this is an improvement.

Verdict: APPROVE (cannot self-approve via GitHub, posting as comment)

No architectural concerns.

Copy link
Copy Markdown
Owner Author

@steilerDev steilerDev left a comment

Choose a reason for hiding this comment

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

[ux-designer] Design compliance review for PR #1165 — FAB for mobile sidebar toggle.

Note: verdict would be approve — posting as comment because GitHub blocks self-review approval.

Summary

Solid implementation. Tokens are used comprehensively throughout the new .menuFab styles, dark mode is handled correctly via semantic tokens, touch targets meet the 44×44px minimum, focus-visible uses box-shadow: var(--shadow-focus) (the correct pattern — no outline regression), and prefers-reduced-motion is guarded. The removal of Header.tsx also eliminates two legacy violations (outline: 2px solid var(--color-primary) on focus, hardcoded 0.25rem radius) that were not being enforced before.

Findings

Medium — Breakpoint overlap at 1024px

/* Current */
@media (max-width: 1024px) { … }

The desktop sidebar becomes visible at min-width: 1025px (per the existing AppShell media query). With max-width: 1024px, the FAB is visible at exactly 1024px wide, which is correct, but to prevent ambiguity and match the established pattern, the upper bound for tablet/mobile contexts should be 1023px:

@media (max-width: 1023px) { … }

This is consistent with the breakpoint guidance in the style guide and avoids a one-pixel overlap edge case.

Low — z-index calc vs. existing token

z-index: calc(var(--z-overlay) + 10);  /* = 60 */

--z-sidebar is defined as 100 in tokens.css and is the correct semantic slot for UI elements that must sit above the overlay (50). The FAB is a sidebar-control affordance, so var(--z-sidebar) would be more semantically appropriate and avoids the magic + 10 offset:

z-index: var(--z-sidebar);

These are non-blocking. The breakpoint issue should be addressed before merge; the z-index is a refinement item.

steilerDev pushed a commit that referenced this pull request Mar 22, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Frontend Developer and others added 3 commits March 22, 2026 15:45
Remove the Header component that wasted 60px of vertical space on every
page with no meaningful content. Replace the mobile sidebar toggle with
a floating action button (FAB) in AppShell, visible only on mobile/tablet
viewports.

Fixes #1161

Co-Authored-By: Claude frontend-developer (Haiku) <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester (Sonnet) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The App integration test still referenced role="banner" from the removed
Header component. Update to use data-testid="menu-fab" instead.

Fixes #1161

Co-Authored-By: Claude frontend-developer (Haiku) <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester (Sonnet) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@steilerDev steilerDev force-pushed the fix/1161-remove-empty-header branch from 305f9c6 to 935e2b9 Compare March 22, 2026 14:46
@steilerDev steilerDev merged commit 2775f6a into beta Mar 22, 2026
15 of 31 checks passed
@steilerDev steilerDev deleted the fix/1161-remove-empty-header branch March 22, 2026 14:50
@github-actions
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 2.2.0-beta.20 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 2.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant