Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The breaking layout overhaul has an unresolved rendering issue and needs final human review of browser-validation results.
1 open issue
What changed in this PR
Moves the shared app header into normal flow and replaces React-managed layout offsets with CSS to reduce hydration layout shifts. This breaking change removes the layout-dimensions APIs.
Changes:
- Adds local height measurement, CSS variables, and sticky positioning.
- Reworks page and dictionary layouts around CSS grid.
- Removes obsolete spacing APIs and adds measurement and header tests.
| File | Description |
|---|---|
| tests/useLayoutHeight.test.tsx | Tests height measurement and cleanup. |
| tests/headerPosition.test.tsx | Tests sticky header positioning. |
| src/views/ResearchView/assistant/components/Drawer/drawer.tsx | Removes spacing props. |
| src/views/ResearchView/assistant/components/Drawer/drawer.styles.ts | Adds CSS-based drawer sizing. |
| src/styles/common/mixins/pinned.ts | Adds sticky and focus styles. |
| src/styles/common/mixins/breakpoints.ts | Adds the 1024px upward breakpoint. |
| src/providers/layoutDimensions/types.ts | Removes provider types. |
| src/providers/layoutDimensions/provider.tsx | Removes the dimensions provider. |
| src/providers/layoutDimensions/hook.ts | Removes the dimensions hook. |
| src/providers/layoutDimensions/context.tsx | Removes dimensions context. |
| src/providers/layoutDimensions/constants.ts | Removes default dimensions. |
| src/hooks/UseLayoutSpacing/types.ts | Removes spacing types. |
| src/hooks/UseLayoutSpacing/hook.ts | Removes the spacing hook. |
| src/hooks/useHtmlStyle/types.ts | Supports CSS custom properties. |
| src/hooks/useHtmlStyle/constants.ts | Defines height property names. |
| src/components/Layout/hooks/UseLayoutHeight/utils.ts | Writes observed heights to CSS. |
| src/components/Layout/hooks/UseLayoutHeight/types.ts | Defines measurement results. |
| src/components/Layout/hooks/UseLayoutHeight/hook.ts | Adds local height measurement. |
| src/components/Layout/components/Sidebar/sidebar.styles.ts | Adds size containment. |
| src/components/Layout/components/Sidebar/components/SidebarPositioner/sidebarPositioner.tsx | Removes measured height props. |
| src/components/Layout/components/Sidebar/components/SidebarPositioner/sidebarPositioner.styles.ts | Pins below the header using CSS. |
| src/components/Layout/components/Main/main.tsx | Removes the header offset. |
| src/components/Layout/components/Main/main.styles.ts | Replaces offset styles with StyledMain. |
| src/components/Layout/components/Header/header.tsx | Adds measurement and global scroll padding. |
| src/components/Layout/components/Header/header.styles.ts | Adds scroll-padding and focus styles. |
| src/components/Layout/components/Header/components/Content/components/Navigation/components/NavigationMenu/navigationMenu.styles.ts | Sizes menus using header height. |
| src/components/Layout/components/Header/common/constants.ts | Sets sticky positioning and height fallback. |
| src/components/Layout/components/Footer/footer.tsx | Measures footer height locally. |
| src/components/Layout/components/Footer/footer.styles.ts | Uses the shared height constant. |
| src/components/Layout/components/Footer/common/constants.ts | Defines footer height fallback. |
| src/components/Layout/components/ContentLayout/contentLayout.tsx | Removes measured offset props. |
| src/components/Layout/components/ContentLayout/contentLayout.styles.ts | Uses CSS sticky positioning. |
| src/components/Layout/components/ContentLayout/components/Main/main.tsx | Updates the styled-main import. |
| src/components/Layout/components/AppLayout/appLayout.styles.ts | Introduces the three-row app grid. |
| src/components/Index/indexView.tsx | Removes spacing-hook usage. |
| src/components/Index/indexView.styles.ts | Adds contained, responsive sizing. |
| src/components/Error/error.tsx | Removes measured header spacing. |
| src/components/Error/error.styles.ts | Removes offset styles. |
| src/components/DataDictionary/hooks/UseMeasureFilters/types.ts | Removes obsolete measurement types. |
| src/components/DataDictionary/hooks/UseMeasureFilters/hook.ts | Removes header-dependent filter measurement. |
| src/components/DataDictionary/dataDictionary.tsx | Uses local measurement and grid layouts. |
| src/components/DataDictionary/dataDictionary.styles.ts | Defines grid areas and scroll padding. |
| src/components/DataDictionary/components/Layout/constants.ts | Defines sticky offsets and filter fallback. |
| src/components/DataDictionary/components/Layout/components/TitleLayout/types.ts | Removes spacing props. |
| src/components/DataDictionary/components/Layout/components/TitleLayout/titleLayout.tsx | Updates parameter documentation. |
| src/components/DataDictionary/components/Layout/components/TitleLayout/titleLayout.styles.ts | Pins and truncates desktop titles. |
| src/components/DataDictionary/components/Layout/components/OutlineLayout/types.ts | Removes spacing props. |
| src/components/DataDictionary/components/Layout/components/OutlineLayout/outlineLayout.tsx | Updates parameter documentation. |
| src/components/DataDictionary/components/Layout/components/OutlineLayout/outlineLayout.styles.ts | Uses CSS offsets and footer compensation. |
| src/components/DataDictionary/components/Layout/components/FiltersLayout/types.ts | Removes spacing props. |
| src/components/DataDictionary/components/Layout/components/FiltersLayout/filtersLayout.tsx | Updates parameter documentation. |
| src/components/DataDictionary/components/Layout/components/FiltersLayout/filtersLayout.styles.ts | Adds sticky positioning and collapsible spacing. |
| src/components/DataDictionary/components/Layout/components/EntitiesLayout/types.ts | Removes the spacing property. |
| src/components/DataDictionary/components/Layout/components/EntitiesLayout/entitiesLayout.tsx | Stops forwarding spacing props. |
| src/components/DataDictionary/components/Layout/components/EntitiesLayout/entitiesLayout.styles.ts | Places entities below filters without offsets. |
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The breaking layout migration needs final human review of browser verification and consumer compatibility, particularly sizing, sticky positioning, and focus scrolling.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
…offsets with css (#998) the header was position: fixed, so every page pushed its content down by a js-measured header height. that height was 0 in the server render and only corrected on hydration (a ~57px layout shift on main), and wrapping announcements could leave page titles under the header. - the app bar is position: sticky at every width, in flow, so content follows it at its real height in the server render. announcements stay inside it. - applayout is a grid (header auto / main 1fr / footer auto) with min-height 100vh: the page grows, so the header stays pinned, and height: 100% below main still fills the space between header and footer. - header and footer measure themselves with uselayoutheight, which writes --header-height and --footer-height on html from the resizeobserver, before paint. css reads them with fallbacks (57px / 56px). - the header renders a global html scroll-padding-top of the header height, so hash links, focus and scrollintoview clear it on every page. focusable elements inside pinned regions (header, dictionary title and filters) get scroll-margin-top: -100vh so focusing them never jumps the page. - main, contentlayout, error, indexview and the research drawer no longer offset by the header height. sticky elements pin at top: var(--header-height) through a shared pinned() style. - the explore view, permanent sidebar and research drawer use contain: size to fill the main row without growing it. - the data dictionary is a grid of title / outline / filters / entities areas with a fixed 74px title row (long titles truncate; #1134 adds anchor positioning). entities follow the filters with no measured padding, scroll-padding is server-rendered, an empty filters band collapses, and the outline keeps its footer compensation via var(--footer-height). breaking changes: - removed layoutdimensionsprovider, uselayoutdimensions and the providers/layoutdimensions module (dimensions.header/footer.height, headerref, footerref). - removed uselayoutspacing and the layoutspacing type. - removed header-offset props from styled components: headerheight on contentgrid, navigationgrid, outlinegrid and positioner; offset on errorlayout; spacing props on styledgridentityview, the research drawer and the data dictionary layouts. - removed mainwithoffset, and renamed the styled export in main/main.styles from main to styledmain. - the header is in flow, so any offset a consumer adds from the header height now doubles. migration steps are in #998. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ild (#998) with emotion's default inline server rendering, a <style> element can precede the filters grid inside the filters layout, so `> :first-child` matched the style element and the grid lost its top margin until hydration moved the styles, shifting the content. the filters grid now carries a stable `data-dictionary-filters` class (FILTERS_CLASS_NAME, following MARKDOWN_CLASS_NAME), and the filters layout targets it for its top margin and the empty-band rule. the bottom margin stays on `> :last-child`, which inline styles never affect. the emotion unsafe-selector test now covers the filters layout. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
frano-m
force-pushed
the
fran/998-in-flow-header
branch
from
October 22, 2026 06:42
899aa72 to
f4842e1
Compare
… in context (#998) the header now sets html scroll-padding-top to its height, and scrollintoview centres within the viewport less that padding. showfigure only subtracted the figure's own scroll-margin-top when deciding whether a figure is too tall to centre, so a figure taller than the window less the header was centred and its top ended up behind the sticky header. it now subtracts the computed scroll-padding-top of html as well. a test covers a figure that fits the window but not the window less the header. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
frano-m
marked this pull request as ready for review
October 24, 2026 02:29
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #998.
Moves the app header into normal flow (
position: sticky) and replaces every JS-measured layout offset with CSS. Content now follows the header at its real height in the server-rendered HTML. The ~57px hydration layout shift onmain(clevercanary/hca-atlas-tracker#1543) is gone, and wrapping announcements can no longer leave page titles under the header.This is a breaking change (
feat!). It removes the layout-dimensions provider and API. Migration steps are in #998.What changed
position: stickyat every width, announcements still inside. A server-rendered globalhtml { scroll-padding-top: var(--header-height) }lets hash links, focus andscrollIntoViewclear it on every page.HeaderandFootermeasure themselves withuseLayoutHeight, which writes--header-height/--footer-heighton<html>from theResizeObservercallback, before paint and without re-rendering. CSS reads them with fallbacks (57px / 56px).auto 1fr auto) withmin-height: 100vh. The page grows, so the header stays pinned, andheight: 100%belowmainstill fills the space between header and footer.Main,ContentLayout,Error,IndexViewand the ResearchView drawer no longer push content down by the header height. Sticky elements pin attop: var(--header-height)through a sharedpinned()style.scroll-margin-top: -100vh. Focusing them, e.g. a menu restoring focus to its trigger, no longer jumps the page.contain: size, so they fill the main row without growing it. The page fits the window frommdup; belowmdthe explore view is100vh - headerand the footer follows.title / outline filters / outline entities) with a fixed 74px title row; long titles truncate (Use CSS anchor positioning to pin the data dictionary outline below a wrapping title #1134 lets them wrap)var(--footer-height)Breaking changes
LayoutDimensionsProvider,useLayoutDimensionsandproviders/layoutDimensions(dimensions.header/footer.height,headerRef,footerRef).useLayoutSpacingand theLayoutSpacingtype.headerHeightonContentGrid,NavigationGrid,OutlineGridandPositioner;offsetonErrorLayout; spacing props onStyledGridEntityView, the research drawer and the data dictionary layouts.MainWithOffset, and renamed the styled export inMain/main.stylesfromMaintoStyledMain.Verification
All nine consumers (anvil-portal, brc-analytics, data-browser, data-portal, hca-atlas-tracker, hprc-data-explorer, meta-disco-browser, ncpi-dataset-catalog, ucsc-brain-explorer) were captured before and after, in five rounds:
PerformanceObserver, deep-link landings and focus probesEach consumer migrated in a scratch worktree, and its typecheck and lint pass. Each has a
migration.patchready for its own follow-up PR.MAINhydration shift is gone, and CLS is the same or lower everywhere (e.g. brc home 1.00 → 0.009 after migration).test-compile,lint,check-format,tscbuild and 886 tests pass. New tests coveruseLayoutHeight(writes and removes the variable, including a measured 0) and the header'sposition: sticky.Follow-ups
@supportsfallback to this layout), then Remove the data dictionary's anchor positioning @supports fallback once browser support is confirmed #1135 to remove the fallback.migration.patch.🤖 Generated with Claude Code