Skip to content
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

Merged
merged 17 commits into from
Apr 15, 2021

Conversation

stevenhansel
Copy link

@stevenhansel stevenhansel commented Mar 24, 2021

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 in docusaurus-theme-classic (for now), we can organize the theme class names much more concise.

I've added ThemeClassNames.ts in docusaurus-theme-common which contains the stuff that I've described before, and I applied it in all the pages in docusaurus-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.)

lisa761 and others added 5 commits February 15, 2021 19:39
[pull] master from facebook:master
[pull] master from facebook:master
[pull] master from facebook:master
[pull] master from facebook:master
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 24, 2021
@netlify
Copy link

netlify bot commented Mar 24, 2021

@netlify
Copy link

netlify bot commented Mar 24, 2021

@github-actions
Copy link

github-actions bot commented Mar 24, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 82
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4511--docusaurus-2.netlify.app/

Copy link
Collaborator

@slorber slorber left a 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)

@slorber slorber linked an issue Mar 29, 2021 that may be closed by this pull request
4 tasks
@slorber slorber changed the title chore: allow global styling in Layout component by adding pageClassName props feat(v2): add unique page/wrapper className to each theme pages Mar 29, 2021
@stevenhansel
Copy link
Author

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:

  • Blog Pages, which includes (BlogListPage, BlogPostPage, BlogTagsListPage, BlogTagsPostPage)
  • DocPage
  • MDXPage

I have also added the wrapperClassName constants in docusaurus-theme-common and applied it to the previous pages.
Apparently, the wrapperClassName in MDXPage is passed by props, so I'm not quite sure if I should change that or not. So, for right now I'm leaving it be.

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?

Comment on lines 18 to 19
blogPage: 'main-docs-wrapper',
docPage: 'blog-wrapper',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
blogPage: 'main-docs-wrapper',
docPage: 'blog-wrapper',
blogPage: 'blog-wrapper',
docPage: 'main-docs-wrapper',

Looks interverted here

Copy link
Collaborator

@slorber slorber Apr 1, 2021

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"?

Copy link
Author

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!

Copy link
Collaborator

@slorber slorber left a 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)

@slorber
Copy link
Collaborator

slorber commented Apr 1, 2021

For MDX pages the wrapper can be set with frontmatter, but a solution could be to do something like:

      wrapperClassName={wrapperClassName ?? ThemeClassNames.wrappers.mdxPages}>

@stevenhansel
Copy link
Author

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)

Thank you for the comments & reviews. I have resolved all of the reviews that were given. I have added the documentation under the Global Styles section in which I explained briefly about the purpose of the Theme Class Names but now how and where to use them, do I need to explain them too?

Also about this:

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)

Do you mean adding a comment in the code file or in the documentation page?
Screen Shot 2021-04-05 at 19 54 26

Copy link
Collaborator

@slorber slorber left a 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 = {
Copy link
Collaborator

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 Show resolved Hide resolved
`@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 = {
Copy link
Collaborator

@slorber slorber Apr 6, 2021

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';

Copy link
Author

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.

Copy link
Author

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',
Copy link
Contributor

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.

Suggested change
docPages: 'main-docs-wrapper',
docPages: 'docs-wrapper',

Copy link
Collaborator

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?

Copy link
Contributor

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.

@slorber
Copy link
Collaborator

slorber commented Apr 15, 2021

thanks 👍

@slorber slorber merged commit cd47d8a into facebook:master Apr 15, 2021
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot differentiate BlogListPage and BlogPostPage for styling
6 participants