Skip to content

fix(docsite): present SideNav family Overview preview at intrinsic height#3577

Draft
cixzhang wants to merge 2 commits into
mainfrom
navi/fix/issue-2703-sidenav-preview-align
Draft

fix(docsite): present SideNav family Overview preview at intrinsic height#3577
cixzhang wants to merge 2 commits into
mainfrom
navi/fix/issue-2703-sidenav-preview-align

Conversation

@cixzhang

@cixzhang cixzhang commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Problem

On the component Overview tab, the SideNav family renders top-aligned with a
large empty gap below
— it looks broken rather than centered/sized as built.

Affects the shared family:

  • Side Nav
  • Side Nav Item
  • Side Nav Section
  • Side Nav Collapse Button
  • (Side Nav Heading — same showcase pattern, same root cause)

Root cause

ShowcasePreview renders the Overview preview inside a flex box with a fixed
aspect-ratio: 16 / 9
and align-items: center. That works for
content-sized components, but the SideNav root is a full-height sidebar
(display: flex; flex-direction: column; height: 100%) with an inner
flex: 1 scroll region.

Because the preview box has a definite height (from the aspect ratio), the
sidebar's height: 100% resolves against it and stretches to fill the box.
align-items: center is a no-op once the child already spans the full height.
Inside the sidebar, the header/sections sit at the top and the flex: 1 region
expands into the remaining space — so the content reads as "pinned to the top"
with an empty gap below. This is the reported appearance; it is not how a real
sidebar (which fills a viewport-height rail) is meant to look in a short,
wide preview tile.

Fix

Treat the SideNav family as full-height in the preview and present it at its
intrinsic height, top-anchored instead of stretched-and-centered:

  • Give the preview container an auto height (maxHeight cap) instead of the
    fixed aspect-ratio. With an indefinite container height, the sidebar's
    height: 100% computes to auto → the sidebar is content-sized, so there is
    no empty gap.
  • Anchor with align-items: flex-start, matching how a sidebar reads (top-down).

This is the smallest correct change and keeps align-items: center +
aspect-ratio: 16 / 9 for every other component. The existing SideNav-only
onClickCapture guard is generalized to the whole family in the same place.

Validation

Chosen method: typecheck + lint + existing preview test suite + a documented
docsite eyeball
— not a bespoke string-pinning test. The bug is a pure
flexbox layout issue in the preview container; there is no untested code
behavior that warrants a new assertion, and the fix's correctness is a rendered
result best confirmed visually.

  • apps/docsite typecheck: no errors in the changed file.
  • Lint: clean.
  • Existing preview tests (component-preview-state, component-preview-theme):
    15/15 pass — no regression.
  • Root cause verified by CSS mechanism: a height: 100% flex child fills a
    definite-height container regardless of align-items, and resolves to auto
    under an indefinite-height container.

What to eyeball in the docsite (Overview tab): open Side Nav, Side Nav Item,
Side Nav Section, and Side Nav Collapse Button. The sidebar should sit snug in
the preview at its natural height (no large empty area below), anchored to the
top. Confirm all other components' Overview previews are unchanged (still
centered in a 16:9 tile).

Closes #2703

The Overview preview box uses a fixed 16/9 aspect ratio with align-items:
center. The SideNav family is a full-height sidebar (height: 100%), so it
stretches to fill the box and reads as top-aligned with an empty gap below.

Give the full-height family an auto-height, top-anchored container so
height: 100% resolves to auto (content-sized) and the sidebar presents as
built. All other components keep the centered 16/9 tile.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 4, 2026
@vercel

vercel Bot commented Jul 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview, Comment Jul 5, 2026 2:16am

Request Review

@cixzhang

cixzhang commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

This isn't the right fix. Sidenav is intended to span the height of the container. Maybe the best thing to do is to give the sidenav showcase a clear container like a card and give the container a reasonable height inside the showcase, potentially allowing for scroll like the toolbar showcase. We could also include some footer items in the showcase so there's better containment.

@cixzhang

cixzhang commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Understood — you're right that special-casing the SideNav family in ShowcasePreview is the wrong layer. SideNav is meant to span its container's height; the preview box shouldn't fight that.

Your proposed direction makes sense to me, so to confirm the plan before I rework it:

  1. Revert the ShowcasePreview.tsx change entirely (drop the FULL_HEIGHT_COMPONENTS set and the top-anchor/intrinsic-height branch) so the preview box stays uniform.
  2. Fix it in the SideNav showcase block instead: wrap the nav in a clear bounded container (a Card, matching the ToolbarCardHeader pattern) with a reasonable fixed height and overflow for scroll, so height: 100% on the SideNav resolves against a real bounded parent rather than the ratio box.
  3. Add footer items to the showcase (e.g. settings / account at the bottom via the SideNav footer slot) so the nav visibly fills top-to-bottom and the containment reads correctly.

One open question: this only naturally covers the SideNav showcase itself — the sibling family previews (SideNavItem, SideNavSection, SideNavHeading, SideNavCollapseButton) currently lean on the same preview branch. Do you want each of those showcases wrapped in the same bounded Card container too, or is it enough to fix the main SideNav showcase and let the smaller ones sit at their natural size?

Happy to implement once you confirm the container height/scroll and whether to extend it to the sibling showcases.

@cixzhang

cixzhang commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Let's keep it consistent for the family of side nav showcases. The examples should likely avoid the extra container though since we don't want to promote the use of card with side nav. Maybe a better container to use would be AppShell with mobile nav disabled.

Address review: drop the ShowcasePreview special-casing that top-anchored the
SideNav family at intrinsic height (SideNav is meant to span its container).
Instead give each family showcase (SideNav, SideNavItem, SideNavSection,
SideNavCollapseButton, SideNavHeading) a real full-height container by wrapping
it in an AppShell with mobile nav disabled, plus footer/support items so the
rail reads as properly contained top-to-bottom. Revert ShowcasePreview to its
prior generic behavior.
@cixzhang

cixzhang commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Done in 95f378b. Reverted the ShowcasePreview special-casing and moved the fix into the showcases themselves — no Card, consistent across the family.

Each SideNav family showcase (SideNav, SideNavItem, SideNavSection, SideNavCollapseButton, SideNavHeading) is now wrapped in an AppShell with mobileNav={false} and height: 100%, so the rail spans the shell's full height as intended and reads as a real app layout rather than a stretched, top-anchored sidebar. Added support/footer items to the main showcase for better containment.

SideNavHeading was the one edge case — it compares two heading configs, so I rendered it as two side-by-side AppShells (each mobileNav={false}) to keep the comparison while still giving each rail a proper container.

Docsite typechecks clean against the family showcases and the data-extraction suite passes. Worth an eyeball in the docsite preview to confirm the AppShell fills the ratio box the way you're picturing.

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

No new or modified components detected.

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.6KB 0B

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Jul 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [bug-bash] Side Nav (+ Collapse Button, Item, Section): Overview preview appears top-aligned (not how component is built)

2 participants