Skip to content

feat(nav): reveal toolbar on page change, drop dashboard-scroller#204

Merged
mairas merged 1 commit into
mainfrom
feat/reveal-toolbar-on-page-change
Jul 4, 2026
Merged

feat(nav): reveal toolbar on page change, drop dashboard-scroller#204
mairas merged 1 commit into
mainfrom
feat/reveal-toolbar-on-page-change

Conversation

@mairas

@mairas mairas commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Part of the navigation UX redesign (epic #183). Supersedes #195.

Why

The transient dashboard-scroller showed page position as a vertical column of dots on the right edge — a leftover from when page nav was a vertical motion. Page nav is now horizontal, so the dot stack reads wrong (#195 asked to reorient it).

Rather than reorient a second indicator, this consolidates onto the one the toolbar already has. The auto-hiding toolbar's page-nav-control strip shows every page as its own icon in a horizontal row with the active one highlighted — a horizontal cue that matches horizontal nav, with larger touch targets. (Note: the strip is icon-only; the page name is on the icon's aria-label, not rendered as text. For pages sharing the default icon the cue is position-only, like the dots but horizontal.)

What

  • app.component.ts: an effect reveals the toolbar whenever activeDashboard() changes to a non-null page. Driving it off the signal covers swipe, hotkey, tap and remote navigation uniformly (all route through it), matching the old scroller's reach.
  • Removed the dashboard-scroller component (ts/html/scss/spec) and its usage + import in dashboard.component.
  • Tests: three cases in app.component.spec.ts — reveal fires on page change, fires again on each subsequent change, and does not fire on a transition to no active page.

Tradeoff

Revealing the full toolbar is heavier feedback than a bare dot row, and it removes the "flip pages with chrome hidden" mode. For a marine MFD, seeing the page position in the toolbar beats a clean screen, so this is the intended trade. The toolbar is an overlay (no grid reflow) and auto-hides on its existing idle timer, preserving the transient behavior #195 wanted to keep. Heavier chrome on passively-viewed remote/repeater displays is a known consequence (see review thread).

Verification

  • npm run lint — clean.
  • app.component.spec.ts 12/12, dashboard.component.spec.ts 13/13 locally (CI Node 20/22/24 is source of truth).

Closes #195.

The auto-hiding toolbar's page-icon strip is now the transient
page-position indicator. An effect reveals the toolbar whenever the
active page changes, covering swipe, hotkey, tap and remote navigation
since all route through the activeDashboard signal.

Removes the dashboard-scroller component, whose vertical column of
position dots read wrong once page navigation became horizontal. The
toolbar's icon strip supersedes it: it names the active page rather than
showing an anonymous, orientation-dependent dot.

Closes #195.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zURudCY3TTRcNd2acknJ9
@mairas mairas force-pushed the feat/reveal-toolbar-on-page-change branch from 6241a26 to b466a82 Compare July 4, 2026 22:30
@mairas

mairas commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Code review — reveal toolbar on page change

Scope: feat/reveal-toolbar-on-page-change vs main (8 files, +35/−160). Intent: replace the transient dashboard-scroller page indicator with a reveal of the auto-hiding toolbar on page change.

Reviewers (5, independent fresh-context): correctness, testing, maintainability, project-standards (always-on) + adversarial (component-deletion + >50 changed lines). Security / api-contract / reliability not applicable — no auth, endpoints, contracts, or error-handling surface.

No P0/P1. The mechanical risks of a component removal were all checked and cleared: no dangling references anywhere (grep-clean repo-wide, dir removed, import + @else branch gone), no reveal-storm timer leak (ChromeVisibilityService keeps a single idleTimer and clearIdle()s before each re-arm), reveal-while-suppressed doesn't fight the dialog/hover logic (scheduleHide() early-returns while suppressCount > 0), and reveal() before the toolbar mounts is safe (touches only signals/timers). Effect placement in app.component.ts is correct — it already owns chrome intent and injects both services; pushing the effect into ChromeVisibilityService would add an unwanted dependency edge.

P2 — Moderate (fixed in this review cycle)

# File Issue Reviewer(s) Conf
1 app.component.spec.ts The does not reveal… test set activeDashboard null→null — a no-op on a signal already at null, so the effect never re-ran and the === null guard was never exercised. The test passed even with the guard deleted (false confidence). testing, maintainability, correctness, adversarial 0.90

P3 — Low

# File Issue Reviewer(s) Conf
2 app.component.spec.ts No coverage that reveal fires again on a subsequent change (only null→1 tested). A "reveal once" latch regression would pass. testing 0.75
3 app.component.ts:121 Returning from /settings to the same page no longer shows a page cue: activeDashboard is unchanged so the effect doesn't fire, and if the idle timer already elapsed the toolbar stays hidden. The old scroller re-animated on every dashboard re-mount. adversarial 0.65

Applied fixes

Design considerations (author's call — not defects)

These are behavioral differences from the removed scroller. The PR already owns the "heavier feedback" tradeoff; these sharpen three specific points:

  • Page name is not shown. page-nav-control renders iconOnly=true with the page name bound to aria-label only. The visible cue is a highlighted icon, clear when pages have distinct icons but position-only when they share the default dashboard-dashboard icon. The old scroller rendered the page-name text. (PR description corrected — it claimed the strip "names the page".)
  • Remote/repeater displays. Remote page changes route through activeDashboard, so a passively-viewed display driven from another device now pops the full interactive toolbar for 4s over marine data on each remote change, vs the old lightweight ~1.35s non-interactive fade.
  • Return-to-same-page ([PCS-01] Missing-config recovery broken: 200 {} from SK triggers no recovery prompt #3 above): no cue when re-entering a dashboard on the previously-active page. Fixable by also revealing on NavigationEnd into a dashboard route (already subscribed for dashboardVisible), if a re-entry cue is wanted.

Coverage

  • Same-value no-refire is not a regression — the old scroller's input also deduped on equality, and setActiveDashboardIndex guards itemIndex === activeDashboard().
  • Deleting dashboard-scroller.component.spec.ts lost no behavioral coverage — it only asserted toBeTruthy().
  • Widening the spec stub's activeDashboard to signal<number|null>(null) is safe and now matches the real service signature; no other test in the file reads it.
  • Per repo CLAUDE.md, CI (Node 20/22/24) is the source of truth for specs — the local 12/12 green needs CI confirmation.

Verdict: Ready to merge

The one actionable defect (#1) was fixed in-cycle. #3 and the design considerations are the author's call and don't block; none are correctness bugs.

@mairas mairas merged commit d77e38f into main Jul 4, 2026
7 checks passed
@mairas mairas deleted the feat/reveal-toolbar-on-page-change branch July 4, 2026 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(nav): dashboard-scroller page indicator is oriented vertically; page nav is now horizontal

1 participant