You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Code review of #234 (see issue comments / PR review) flagged that src/lib/layout-constants.ts still has two forms of "magic number that can silently drift":
TIMELINE_TOOLBAR_HEIGHT_PX ({ mobile: 63, desktop: 63 }) is a hand-measured guess at the toolbar's rendered height (padding + border + content) — TimelineToolbar.tsx has no explicit height class, so this number can drift silently whenever the toolbar's content changes (font-size, chip wrapping, added/removed controls). This is exactly the "empirically derived, margin-collapse-dependent spacing" Timeline sticky chrome: toolbar offset, sticky header strip, unboxed timeline #234 asked to eliminate. HEADER_STRIP_TOP_PX is derived from it, so drift here also breaks the header strip's docking.
TOP_BAR_HEIGHT_PX ({ mobile: 64, desktop: 80 }) is backed by an explicit, fixed Tailwind class in TopBar.tsx (h-16 md:h-20) — but that fact is duplicated by hand as STICKY_TOP_BELOW_TOP_BAR_CLASS = "top-16 md:top-20". The px map and the class string have to be kept in sync manually; nothing enforces it.
What to do
For TIMELINE_TOOLBAR_HEIGHT_PX, pick one:
(a) Measure the toolbar's real height at runtime (e.g. a ResizeObserver-backed hook, sibling to useScrollLeft/useScrollEdgeFade), and pass it down to TimeScaleContainer instead of a constant — accurate under any content change, some added complexity.
(b) Pin the toolbar to an explicit fixed height (matching the TOP_BAR_HEIGHT_PX pattern) — simpler, but only safe if the toolbar's content is guaranteed single-line/non-wrapping at every breakpoint and text-scale; verify that before choosing this.
For TOP_BAR_HEIGHT_PX / STICKY_TOP_BELOW_TOP_BAR_CLASS: derive one from the other (e.g. generate the Tailwind class string from the px map, or vice versa) so there's a single source of truth instead of two hand-synced representations.
Acceptance criteria
TIMELINE_TOOLBAR_HEIGHT_PX/HEADER_STRIP_TOP_PX can no longer silently drift from the toolbar's actual rendered height
TOP_BAR_HEIGHT_PX and STICKY_TOP_BELOW_TOP_BAR_CLASS have one source of truth
No visual regression in toolbar/header-strip docking at mobile and desktop breakpoints
Parent
Spec: #234
Context
Code review of #234 (see issue comments / PR review) flagged that
src/lib/layout-constants.tsstill has two forms of "magic number that can silently drift":TIMELINE_TOOLBAR_HEIGHT_PX({ mobile: 63, desktop: 63 }) is a hand-measured guess at the toolbar's rendered height (padding + border + content) —TimelineToolbar.tsxhas no explicit height class, so this number can drift silently whenever the toolbar's content changes (font-size, chip wrapping, added/removed controls). This is exactly the "empirically derived, margin-collapse-dependent spacing" Timeline sticky chrome: toolbar offset, sticky header strip, unboxed timeline #234 asked to eliminate.HEADER_STRIP_TOP_PXis derived from it, so drift here also breaks the header strip's docking.TOP_BAR_HEIGHT_PX({ mobile: 64, desktop: 80 }) is backed by an explicit, fixed Tailwind class inTopBar.tsx(h-16 md:h-20) — but that fact is duplicated by hand asSTICKY_TOP_BELOW_TOP_BAR_CLASS = "top-16 md:top-20". The px map and the class string have to be kept in sync manually; nothing enforces it.What to do
For
TIMELINE_TOOLBAR_HEIGHT_PX, pick one:ResizeObserver-backed hook, sibling touseScrollLeft/useScrollEdgeFade), and pass it down toTimeScaleContainerinstead of a constant — accurate under any content change, some added complexity.TOP_BAR_HEIGHT_PXpattern) — simpler, but only safe if the toolbar's content is guaranteed single-line/non-wrapping at every breakpoint and text-scale; verify that before choosing this.For
TOP_BAR_HEIGHT_PX/STICKY_TOP_BELOW_TOP_BAR_CLASS: derive one from the other (e.g. generate the Tailwind class string from the px map, or vice versa) so there's a single source of truth instead of two hand-synced representations.Acceptance criteria
TIMELINE_TOOLBAR_HEIGHT_PX/HEADER_STRIP_TOP_PXcan no longer silently drift from the toolbar's actual rendered heightTOP_BAR_HEIGHT_PXandSTICKY_TOP_BELOW_TOP_BAR_CLASShave one source of truth