fix(theme): preserve sidebar height on collapse by 0916dhkim · Pull Request #8328 · facebook/docusaurus

0916dhkim · 2022-11-12T16:59:42Z

Pre-flight checklist

I have read the Contributing Guidelines on pull requests.
If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Test Plan

2022-11-12.12-05-41.mp4

Test links

Deploy preview: https://deploy-preview-8328--docusaurus-2.netlify.app/tests/docs
Same page on prod: https://docusaurus.io/tests/docs

Related issues/PRs

#4415

netlify · 2022-11-12T17:02:50Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`71300e2`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/636fd0ff64e5d200084c88c6
😎 Deploy Preview	https://deploy-preview-8328--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-11-12T17:03:39Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 63	🟢 97	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 81	🟢 100	🟢 100	🟢 100	🟢 90	Report

0916dhkim

Summary of changes I made:

0916dhkim · 2022-11-12T17:11:50Z

packages/docusaurus-theme-classic/src/theme/DocRoot/Layout/Sidebar/index.tsx

+        <div
+          className={clsx(
+            styles.sidebarViewport,
+            hiddenSidebar && styles.sidebarViewportHidden,
+          )}>
+          <DocSidebar


Wrap the .sidebar element with a new .sidebarViewport element.

0916dhkim · 2022-11-12T17:13:24Z

packages/docusaurus-theme-classic/src/theme/DocRoot/Layout/Sidebar/styles.module.css

+  .sidebarViewport {
+    top: 0;
+    position: sticky;
+    height: 100%;
+    max-height: 100vh;
+  }


.sidebarViewport is a sticky element that's clamped to screen height. Its width will shrink on collapse.
.sidebarViewport, .sidebar, and .expandButton elements will have the same height (on collapse & expand).

0916dhkim · 2022-11-12T17:14:57Z

packages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/styles.module.css

-    height: 0;
-    overflow: hidden;
    visibility: hidden;
  }


.sidebar will not shrink in size on collapse anymore. It will just become invisible. This preserves the height.

thanks, looks better indeed 👍

Wonder if it affects accessibility and keyboard navigation? Is it possible now to navigate with the keyboard (tabs) on these hidden elements?

@slorber That's a good point. I haven't tested keyboard navigation on hidden elements.

0916dhkim · 2022-11-12T17:17:26Z

packages/docusaurus-theme-classic/src/theme/DocRoot/Layout/Sidebar/index.tsx

+            onCollapse={toggleSidebar}
+            isHidden={hiddenSidebar}
+          />
+          {hiddenSidebar && <ExpandButton toggleSidebar={toggleSidebar} />}


Put .expandButton inside .sidebarViewport (outside .sidebar).

lex111

Brilliant solution 👍 , I really shouldn't have hidden the sidebar content when collpasing, that was the cause of this bug.

slorber

Thanks, that looks like a good solution 👍

I noticed that our dogfood sidebar with ads on top/bottom now produce some weird behavior, can you take a look please?

It seems related to the presence of the announcementBar

slorber · 2022-11-24T11:50:26Z

packages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/styles.module.css

-    height: 0;
-    overflow: hidden;
    visibility: hidden;
  }


thanks, looks better indeed 👍

Wonder if it affects accessibility and keyboard navigation? Is it possible now to navigate with the keyboard (tabs) on these hidden elements?

slorber · 2022-11-24T11:56:55Z

Hmmm, nevermind, can also reproduce this issue on prod 😅 : https://docusaurus.io/tests/docs

slorber · 2022-11-24T13:37:22Z

This doesn't improve that case:

But that's a separate issue, will see if I can fix it in another PR

slorber · 2022-11-24T13:49:22Z

LGTM thanks 👍

fix: preserve sidebar height on collapse

71300e2

facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 12, 2022

0916dhkim commented Nov 12, 2022

View reviewed changes

0916dhkim marked this pull request as ready for review November 12, 2022 17:19

0916dhkim requested a review from slorber as a code owner November 12, 2022 17:19

0916dhkim mentioned this pull request Nov 12, 2022

Preserve sidebar height when toggling sidebar #4415

Closed

lex111 reviewed Nov 13, 2022

View reviewed changes

slorber reviewed Nov 24, 2022

View reviewed changes

slorber added the pr: bug fix This PR fixes a bug in a past release. label Nov 24, 2022

slorber changed the title ~~fix: preserve sidebar height on collapse~~ fix(theme): preserve sidebar height on collapse Nov 24, 2022

slorber merged commit 1bf0589 into facebook:main Nov 24, 2022

slorber mentioned this pull request Nov 24, 2022

Docs layout issue: content container does not grow to take all the available space #8369

Closed

7 tasks

slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Jan 26, 2023

slorber pushed a commit that referenced this pull request Jan 26, 2023

fix(theme): preserve sidebar height on collapse (#8328)

d20d144

slorber added backported This PR has been backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Jan 27, 2023

slorber mentioned this pull request Oct 19, 2023

chore: v3.0.0-rc.0 release #9418

Merged

Uh oh!

Comments

Conversation

0916dhkim commented Nov 12, 2022 • edited by slorber Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-flight checklist

Motivation

Test Plan

Test links

Related issues/PRs

Uh oh!

netlify bot commented Nov 12, 2022

✅ [V2]

Uh oh!

github-actions bot commented Nov 12, 2022

⚡️ Lighthouse report for the deploy preview of this PR

Uh oh!

0916dhkim left a comment

Choose a reason for hiding this comment

Uh oh!

0916dhkim Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

slorber Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

lex111 left a comment

Choose a reason for hiding this comment

Uh oh!

slorber left a comment

Choose a reason for hiding this comment

Uh oh!

slorber Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

slorber commented Nov 24, 2022

Uh oh!

slorber commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slorber commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0916dhkim commented Nov 12, 2022 •

edited by slorber

Loading

slorber commented Nov 24, 2022 •

edited

Loading