-
-
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
feat(v2): add unique page/wrapper className to each theme pages #4511
feat(v2): add unique page/wrapper className to each theme pages #4511
Conversation
[pull] master from facebook:master
[pull] master from facebook:master
[pull] master from facebook:master
[pull] master from facebook:master
[V1] Deploy preview failure Built without sensitive environment variables with commit f3c72dc https://app.netlify.com/sites/docusaurus-1/deploys/60649ad3d3a1a0000759b49b |
[V2] Built without sensitive environment variables with commit 0c92a20 |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4511--docusaurus-2.netlify.app/ |
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.
Thanks, that looks nice
We need:
- the same for all doc pages (not just blog pages)
- the same for "wrappers" like "blog-wrapper" / "docs-wrapper" etc (keep it retrocompatible, just add missing wrappers + move names to
ThemeClassNames.wrappers
) - maybe find a way to document this? (not sure where, try to suggest a good place if you think about one)
Thanks, just resolved the issues, just making sure the pages that I have to change are:
I have also added the Other than that, all left is adding the documentation regarding this change, apparently I have no clue regarding where to add this topic in the docs, do you have any hints? |
blogPage: 'main-docs-wrapper', | ||
docPage: 'blog-wrapper', |
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.
blogPage: 'main-docs-wrapper', | |
docPage: 'blog-wrapper', | |
blogPage: 'blog-wrapper', | |
docPage: 'main-docs-wrapper', |
Looks interverted 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.
also what about "blogPages" and "docsPages"?
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.
Woops, I must be tripping, fixing it right now!
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 looks fine, just added some comments.
You can probably add a section here: https://docusaurus.io/docs/styling-layout
To ensure that the doc remains up to date, you can maybe just display ThemeClassNames
file content in a code block? (copy what I did here: https://docusaurus.io/docs/i18n/crowdin#example-configuration)
Also what about adding a comment to ThemeClassNames to describe the purpose of this? (having stable classnames shared by all themes and meant to be targeted by custom CSS)
For MDX pages the wrapper can be set with frontmatter, but a solution could be to do something like:
|
Thank you for the comments & reviews. I have resolved all of the reviews that were given. I have added the documentation under the Also about this:
Do you mean adding a comment in the code file or in the documentation page? |
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.
Thanks, that looks nice, just added a few comments
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
export const ThemeClassNames = { |
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.
I mean add a little // comment here so that someone reading the code understands why we created this file in the first place. Nothing too complicated, just 1 line
website/docs/styling-layout.md
Outdated
`@docusarus/theme-common` provides some predefined CSS class names to provide access for developers to style layout of a page globally in Docusaurus. The purpose is to have stable classnames shared by all themes that are meant to be targeted by custom CSS. | ||
|
||
```ts | ||
export const ThemeClassNames = { |
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.
If we refactor ThemeClassNames
isn't there a risk to forget to update this code snippet, and we end up with a code snippet and actual classnames not in sync over time?
That's why I suggested using something similar to the Crowdin config here, to ensure we display in the code block the real code, so that it stays in sync automatically.
### Example configuration {#example-configuration}
The **Docusaurus v2 configuration file** is a good example of using versioning and multi-instance:
import CrowdinConfigV2 from '!!raw-loader!@site/../crowdin-v2.yaml';
import CodeBlock from '@theme/CodeBlock';
<CodeBlock className="language-yaml" title="test">
{CrowdinConfigV2.split('\n')
// remove comments
.map((line) => !line.startsWith('#') && line)
.filter(Boolean)
.join('\n')}
</CodeBlock>
https://docusaurus.io/docs/i18n/crowdin#example-configuration
You can probably use something like this to import the file content as a string, and display it in a code block:
import ThemeClassNamesCode from '!!raw-loader!@theme-common/src/utils/ThemeClassNames.ts';
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.
Ah, I see. I didn't get what you meant before, going to import it from that file now.
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.
I have updated the documentation and imported the code by using raw-loader
but right now I imported it like this:
import ThemeClassNamesCode from '!!raw-loader!@site/../packages/docusaurus-theme-common/src/utils/ThemeClassNames.ts'; import CodeBlock from '@theme/CodeBlock';
Is there a way to import it from @theme-common
? because I tried before and it couldn't find the file.
wrapper: { | ||
main: 'main-wrapper', | ||
blogPages: 'blog-wrapper', | ||
docPages: 'main-docs-wrapper', |
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.
Let's remove the redundant prefix here for more consistency.
docPages: 'main-docs-wrapper', | |
docPages: 'docs-wrapper', |
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.
@lex111 we already have main-docs-wrapper
and it is used in a css selector by ourselves + other sites already, so this would be a breaking change.
I'd be happy to normalize those wrapper names, but maybe we can handle the breaking changes in an upcoming PR that focus only on the breaking changes?
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.
Ah yes, agree with you, I don't think that such BC will affect many people, especially since this is a relatively new addition, so now is good time to normalize it.
thanks 👍 |
Motivation
It's issued in #4322 that you can't style a page wrapped by a layout component immediately, thus you have to add another wrapper component to put a style around the page itself.
It would be better if we can style the layout component immediately, and by adding another className prop into the layout component, we can achieve this pretty easily.
By adding a constant, called
ThemeClassNames
which contains the page and wrapper class names for every page indocusaurus-theme-classic
(for now), we can organize the theme class names much more concise.I've added
ThemeClassNames.ts
indocusaurus-theme-common
which contains the stuff that I've described before, and I applied it in all the pages indocusaurus-theme-classic
which are:BlogListPage
BlogPostPage
BlogTagsListPage
BlogTagsPostPage
DocPage
MDXPage
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)