fix(layout): consolidate 2 sidebar layout across content types#1265
Merged
fix(layout): consolidate 2 sidebar layout across content types#1265
Conversation
add --layout-sidebar-padding variable for padding between sidebar content and document content, so the scrollbar isn't pushed up against the content this removes my ugly hack from before where I set a negative right margin on the left sidebar sets --layout-sidebar-padding to 1rem + 2px to make the gap between sidebar content and document content the same as it currently is increases --layout-sidebar-min by 1rem so sidebars content doesn't get any narrower absorbs the --layout-side-padding-min padding into the left of the left sidebar to avoid 2px padding for focus outlines and give us greater flexibility with the sidebar layout in the future does the same on the right of the right sidebar to align the right sidebar scrollbar with the right of the screen on smaller displays
move them up to .reference-layout__sidebar
…from 2-sidebars committed with --no-verify to make diffs almost inverse of each other
.layout__2-sidebars for when the right sidebar is to be hidden on mobile .layout__2-sidebars-inline for when the right sidebar is to be put between content header and body on mobile
otherwise we can't consistently position the overlayed sidebar: scroll to top of page, resize page width to 790px and toggle sidebar to repro keeping both media queries because --screen-layout-no-sidebar may change to be smaller than --screen-small-and-narrower in the future again (we don't want to have to go update other files as we change our min content and sidebar sizes and paddings)
urls for testing each renderer: CurriculumAbout: http://localhost:3000/en-US/curriculum/about-curriculum/ CurriculumDefault: http://localhost:3000/en-US/curriculum/faq/ CurriculumModule: http://localhost:3000/en-US/curriculum/getting-started/soft-skills/ CurriculumOverview: http://localhost:3000/en-US/curriculum/getting-started/
Contributor
|
ba08f2e was deployed to: https://fred-pr1265.review.mdn.allizom.net/ |
LeoMcA
commented
Jan 30, 2026
Member
Author
LeoMcA
left a comment
LeoMcA
left a comment
There was a problem hiding this comment.
Copied review comments from #1154 (review)
| @@ -0,0 +1,141 @@ | |||
| .layout__2-sidebars, | |||
| .layout__2-sidebars-inline { | |||
Member
Author
There was a problem hiding this comment.
from @caugner: [nit] maybe:
Suggested change
| .layout__2-sidebars-inline { | |
| .layout__2-sidebars--inline { |
| @@ -0,0 +1,141 @@ | |||
| .layout__2-sidebars, | |||
Member
Author
There was a problem hiding this comment.
from @caugner: [nit] maybe add comment explaining the different classes, and where they are used
components/generic-layout/server.css
Outdated
Comment on lines
5
to
18
| /* Sidebar */ | ||
| .generic-layout__sidebar { | ||
| grid-area: sidebar; | ||
| padding: 2rem 0; | ||
| padding-block: 2rem; | ||
| } | ||
|
|
||
| /* Content */ | ||
| .generic-layout__content { | ||
| grid-area: body; | ||
| padding: 2rem 0; | ||
| padding-block: 2rem; | ||
| } | ||
|
|
||
| /* TOC */ | ||
| .generic-layout__toc { | ||
| grid-column: toc; | ||
| padding: 2rem 0; | ||
| padding-block: 2rem; | ||
| } |
Member
Author
There was a problem hiding this comment.
from @caugner: [nit] should these blocks be merged now?
| } | ||
|
|
||
| @media (--screen-small-and-narrower) { | ||
| @media (--screen-small-and-narrower) or (--screen-layout-no-sidebar) { |
Member
Author
There was a problem hiding this comment.
from @caugner: does this mean we show placements less often?
Member
Author
There was a problem hiding this comment.
Not really: it moved the breakpoint horizontally by about 25px to avoid this buggy layout:
caugner
approved these changes
Feb 3, 2026
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
3 participants
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.
Split from #1154 to extract the layout refactor/consolidation work without changing the gap between sidebar content and layout content so that can be discussed/approved by content.
Introduces a unified approach to how we do our 2 sidebar layout across docs, curriculum and generic content.
Commits are atomic, and some have lengthy explanations in their commit message: I'd recommend reviewing one by one.
Fixes these issues:
If we like this approach, we should probably migrate all the other layouts using the variables to a class-based system, but it felt like I'd got to a coherent and consistent place with this so stopped here.