refactor: remove dead layouts, extract shared utilities, and deduplicate docs routes#1248
Open
jderochervlk wants to merge 7 commits intomasterfrom
Open
refactor: remove dead layouts, extract shared utilities, and deduplicate docs routes#1248jderochervlk wants to merge 7 commits intomasterfrom
jderochervlk wants to merge 7 commits intomasterfrom
Conversation
- Delete ApiLayout component (unused in production routes) - Inline makeBreadcrumbs into ApiRoute.res (its only consumer) - Remove test file that only tested the dead component - Clean up DocsLayout: remove unused makeBreadcrumbs, NavItem/Category aliases, and StaticContent module type - Extract FrontmatterUtils.getField to replace ~150 lines of duplicated frontmatter switch blocks across 6 route files - Extract TocUtils.buildEntries to replace duplicated TOC construction logic across 4 route files
Add a React Router layout route that renders <NavbarSecondary /> and <Outlet />, eliminating the duplicated <NavbarSecondary /> from 7 child route modules. - Create app/layouts/DocsLayoutRoute.res as a layout route - Nest all docs, API, and syntax-lookup routes under the layout - Remove <NavbarSecondary /> from ApiOverviewRoute, ApiRoute, DocsGuidelinesRoute, DocsManualRoute, DocsReactRoute, SyntaxLookupRoute, and SyntaxLookupDetailRoute - DocsOverview remains a flat route (it uses MainLayout, not the docs sidebar pattern)
- Move categories data directly into ApiOverviewRoute.res - Replace <ApiOverviewLayout.Docs> with <DocsLayout categories theme=#Js> - Update tests to use DocsLayout and MemoryRouter - Delete src/layouts/ApiOverviewLayout.res and .resi
Move from src/layouts/ to src/components/ since it is a self-contained page component, not a layout wrapper. It does not accept children and does not provide structural wrapping like the other layouts do.
DocsLayout now owns both the desktop sidebar (via SidebarLayout.Sidebar) and the mobile sidebar (via NavbarTertiary dialog). Routes pass data instead of constructing sidebar JSX inline. - Add ~breadcrumbs and ~editHref props to DocsLayout - DocsLayout renders NavbarTertiary internally with mobile sidebar - Simplify DocsManualRoute, DocsReactRoute, ApiOverviewRoute, and DocsGuidelinesRoute by removing ~30 lines of duplicated sidebar construction and NavbarTertiary rendering from each - ApiRoute is unchanged (uses a different sidebar structure) - CommunityRoute is unchanged (already handles both sidebars internally)
DocsLayout now renders NavbarTertiary internally, which creates a mobile sidebar dialog. This caused two test issues: - Duplicate element matches: sidebar text appeared in both the desktop sidebar and the mobile dialog. Fixed by scoping queries to the sidebar-content testId. - Screenshot dimension mismatches: NavbarTertiary adds height to the rendered layout. Regenerated all affected reference screenshots. Also fixed BrowserRouter -> MemoryRouter in DocsLayout tests per project conventions.
The previous approach of rendering NavbarTertiary inside DocsLayout placed it in the wrong DOM position (inside the layout content area instead of above it), causing visual bugs on mobile and desktop. Fix by reverting DocsLayout to a simple SidebarLayout wrapper and extracting the duplicated mobile sidebar JSX into a shared DocsSidebar component. Routes keep rendering NavbarTertiary at the correct position (above DocsLayout) but use DocsSidebar to avoid the 25-line copy-paste. - Create src/components/DocsSidebar.res with shared sidebar content - Revert DocsLayout to pre-NavbarTertiary state (remove breadcrumbs, editHref props and NavbarTertiary rendering) - Update DocsManualRoute, DocsReactRoute, ApiOverviewRoute, and DocsGuidelinesRoute to use DocsSidebar with NavbarTertiary - Scope ApiOverviewLayout test sidebar queries to sidebar-content testId to avoid matching pagination links - Regenerate reference screenshots
Cloudflare deploymentDeployement ID: 60eb709e-5357-4f8c-ae10-646db44132bc ⛅️ wrangler 4.63.0 (update available 4.82.1) ✨ Uploading _redirects |
jderochervlk
changed the title
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
1 participant
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.
Note
This work was done with the assistance of AI
A series of refactoring steps that remove dead code, extract shared utilities, and consolidate duplicated layout/sidebar logic across docs route modules. Net result: -374 lines (215 added, 589 removed) across 33 files.
Changes
Dead code removal
ApiLayout(unused in any production route) along with its test filemakeBreadcrumbs,NavItem/Categoryaliases, andStaticContentmodule type fromDocsLayoutApiOverviewLayoutdirectly intoApiOverviewRoute, then delete the standalone layout moduleShared utility extraction
FrontmatterUtils.getField-- replaces ~150 lines of duplicated frontmatterswitchblocks across 6 route filesTocUtils.buildEntries-- replaces duplicated TOC construction logic across 4 route filesDocsSidebarcomponent -- extracts the ~25-line mobile sidebar pattern (used withNavbarTertiary) that was copy-pasted acrossDocsManualRoute,DocsReactRoute,ApiOverviewRoute, andDocsGuidelinesRouteLayout route for shared NavbarSecondary
DocsLayoutRouterenders<NavbarSecondary />+<Outlet />as a React Router layout routeapp/routes.res, removing the duplicated<NavbarSecondary />from 7 child route modulesLandingPageLayout rename
LandingPageLayouttoLandingPageand moved fromsrc/layouts/tosrc/components/-- it is a self-contained page component, not a layout wrapper (it accepts no children)Screenshot changes explained
All 7 regenerated reference screenshots are expected visual changes caused by two test infrastructure improvements, not regressions:
BrowserRouterreplaced withMemoryRouter-- tests now use<MemoryRouter initialEntries=["/docs/manual/api"]>instead of<BrowserRouter>(which defaults to pathname/). This means the sidebar'sisItemActivecheck now correctly matches the "Introduction" nav item, rendering it with the active highlight style. The pagination feature inSidebarLayoutalso finds the current page in the nav items, so a "Stdlib -->" next-page link now renders in the content area. Both behaviors match production; they were simply invisible in the old tests because no route matched.Sidebar queries scoped to
sidebar-contenttestId -- becauseDocsLayoutnow renders sidebar content that can appear in both the desktop sidebar and the mobileNavbarTertiarydialog, queries likegetByText("Stdlib")would match multiple elements. Tests now scope queries toscreen->getByTestId("sidebar-content")to target the desktop sidebar specifically.