-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: make docs page wrapper take full height #7025
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-7025--docusaurus-2.netlify.app/ |
Size Change: +19 B (0%) Total Size: 806 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also what I figured need to happen 👍 We have too many class names on the html
element, and docs-wrapper
, docs-doc-page
, plugin-docs
seem exactly equivalent in all cases. It seems beneficial to at least have something that targets the wrapper element instead. Not sure if it goes against @slorber's intention though
I agree with you, it's even more strange to see the class containing of "wrapper" added to the root element of a page. Such classes have no place in the |
Thanks, I think we can move forward from here |
@@ -77,6 +77,7 @@ export default function DocPage(props: Props): JSX.Element { | |||
/> | |||
<HtmlClassNameProvider | |||
className={clsx( | |||
// TODO: it should be removed from here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change looks fine
I don't think the top-level class should be removed because it's a stable one meant to be used for custom CSS targeting: one identity all the pages of the docs plugin, the other identity a specific page of the docs plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we already have plugin-docs
that's applied to all docs plugin pages, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes
right, I think I kept those for temp easier retrocompat ;) we can keep the todo then
Motivation
Fixes #7018, broken as result changes in https://github.com/facebook/docusaurus/pull/6925/files#diff-efc286976b85125e3056044ed1ef259e5794477ab266e4c6207a68d795f76b4a
Note: this bug will be noticeable if the content area column on the right is smaller by height than the sidebar.
I had to add back the
docs-wrapper
class as it was before, since I guess that's the only available way to make things work.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Create empty Markdown page or via dev tools try to empty content area on any existent page -- the sidebar should be stretched to the footer.
Related PRs