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

fix: insert site custom CSS with highest priority #6227

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Dec 30, 2021

Deprecation notice

themeOptions.customCss is deprecated in favor of siteConfig.styling.css.

customCss will be removed so you'd rather migrate to this new option.

Unlike before, site custom CSS will now be inserted last in the site final stylesheet, leading to site-specific rules being used in priority over Infima and theme styling.

This means that you can likely remove some !important usage from your site custom CSS

It can also lead to unwanted side-effects: a custom site CSS rule that was "inactive" and useless (overridden by an Infima/theme rule) would now become "active"

Motivation

Site CSS is often used for customizations

It should be able to override Infima and theme modules CSS easily, without the need for !important

Site CSS should be inserted last, after Infima/theme modules CSS, so that site CSS rules with same specificity as Infima/theme rules can win and be applied in priority

Note: in some specific cases, site CSS rules might be optimized/deduplicated/merged and may appear at the top of the final stylesheet, but in general they would appear at the end (see discussions in #6222)

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

test CSS insertion order

Related

@slorber slorber added pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: bug fix This PR fixes a bug in a past release. labels Dec 30, 2021
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 30, 2021
@github-actions
Copy link

github-actions bot commented Dec 30, 2021

Size Change: +214 B (0%)

Total Size: 670 kB

Filename Size Change
website/build/assets/css/styles.********.css 102 kB +71 B (0%)
website/build/assets/js/main.********.js 498 kB +143 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 40.1 kB
website/build/index.html 29.6 kB

compressed-size-action

@slorber slorber removed the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Dec 30, 2021
@Josh-Cena Josh-Cena changed the title fix: site custom CSS must be inserted at the end fix: insert site custom CSS with highest priority Dec 30, 2021
@@ -33,6 +37,7 @@ export interface DocusaurusConfig {
// trailingSlash undefined = legacy retrocompatible behavior => /file => /file/index.html
trailingSlash: boolean | undefined;
i18n: I18nConfig;
styling: StylingConfig;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this new styling.css config a good API design @Josh-Cena ?

Is styling a good name?

I'd like later to add new styling options like primary color leading to an attempt to auto-generate a color palette, that we could map to Infima/tailwind CSS vars in theme.

IMHO users shouldn't need to write any CSS to be able to tweak a bit the site colors. v1 had options for that too: https://v1.docusaurus.io/docs/en/site-config.html#colors-object

Note: we already have a config.stylesheets API but it's not exactly the same: this just insert a raw CSS file into the HTML head, and this CSS is never processed/optimized. I suspect it's not widely used because it's documented only in one place: https://docusaurus.io/docs/api/docusaurus-config#stylesheets

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have gone for styles... but styling, sure 👍

config.stylesheets is not only rarely used but also very misleading: when I help build other sites (e.g. https://typescript-eslint.io/) I often hack this field for injecting <link /> tags...

@Josh-Cena
Copy link
Collaborator

Ah, clever, I didn't think of using a synthetic plugin to inject the client module... However this is still not strictly the "highest priority" because it can be overridden by global stylesheets imported in the theme components. I had just been wondering how we should solve it

@slorber
Copy link
Collaborator Author

slorber commented Dec 30, 2021

Ah, clever, I didn't think of using a synthetic plugin to inject the client module...

That didn't work actually 😅 client modules all use require(), and CSS loaded this way always comes before CSS loaded more lazily (and routes are lazy-loaded)

I can't figure a good way to have a proper ordering 😅

However this is still not strictly the "highest priority" because it can be overridden by global stylesheets imported in the theme components. I had just been wondering how we should solve it

Our theme doesn't have global stylesheets atm (CSS modules probably don't quality) but I understand the concern.

The thing is: routes are all lazy-loaded in dynamic imports, and CSS modules are only loaded inside those dynamic routes.

Sometimes there are a few dynamic/static imports layers before we even reach the final CSS, like for CodeBlock

I'm not sure how Webpack determines the order for anything dynamic, this is not very clear and looks quite random 😅

Let me know if you see any potential solution, because I can't.

Maybe we should process such CSS outside of Webpack as a separate build pipeline 🤷‍♂️

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Dec 30, 2021

Don't panic haha that was the same mental train I went down in #5987. We have several solutions if the mini CSS extract plugin can't do what we want:

  • Emit individual CSS files for custom CSS (Re-introduce CSS code splitting #6228)
  • Implement our own plugin to only merge the custom CSS file after mini CSS has done its work (didn't look into the impl of miniCSS so that depends on which lifecycles it uses)
  • Admit that lazy-loaded CSS should have higher priority and discourage using global stylesheets in individual routes

@slorber
Copy link
Collaborator Author

slorber commented Jan 5, 2022

According to this Remix blog post, Next.js has a similar and still unsolved CSS ordering problem: vercel/next.js#16630

It doesn't look so easy to implement correctly so I'd rather delay this breaking change after 2.0

@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Jan 5, 2022
@Josh-Cena
Copy link
Collaborator

@slorber What about we still push this change?

  1. We can make sure the custom CSS comes on top of default styles
  2. We make ourselves a new styles root config option that we can populate in the future
  3. We can make all themes zero-options

@slorber
Copy link
Collaborator Author

slorber commented Jan 12, 2022

@slorber What about we still push this change?

I don't think it's a good idea until we have found a a way to insert the CSS at the right place.

We can make sure the custom CSS comes on top of default styles

We don't exactly know yet what's the best way to do that

Changing CSS ordering is annoying for our users and we don't want to recommend a new option that barely works as intended and then change it again to something that works better: this will break sites CSS even if it's an improvement.

Implement our own plugin to only merge the custom CSS file after mini CSS has done its work (didn't look into the impl of miniCSS so that depends on which lifecycles it uses)

That seems to be the best option to ensure CSS is at the end, but I don't really know how complex this would be to implement.

Processing the CSS file separately can work but we must ensure that hot reloading of CSS still works in dev.

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: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants