fix(theme-classic): fix announcementBar css

[slorber]

Motivation

After merging this PR: #3104

The insertion order of CSS classes became different 🤷‍♂️

This is a temporary fix, the original problem looks related to #3678

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

preview

[netlify]

✔️ [V2]

🔨 Explore the source changes: 9c07903

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6184017e3845d000079f7470

😎 Browse the preview: https://deploy-preview-5873--docusaurus-2.netlify.app

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

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

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

[github-actions]

Size Change: +26 B (0%)

Total Size: 881 kB

ℹ️ View Unchanged

Filename	Size	Change
website/.docusaurus/globalData.json	38.2 kB	0 B
website/build/assets/css/styles.********.css	94 kB	+20 B (0%)
website/build/assets/js/main.********.js	474 kB	0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html	66.1 kB	0 B
website/build/blog/index.html	36.8 kB	0 B
website/build/docs/index.html	44 kB	+3 B (0%)
website/build/docs/installation/index.html	51.5 kB	+3 B (0%)
website/build/index.html	29.5 kB	0 B
website/build/tests/docs/index.html	25.2 kB	0 B
website/build/tests/docs/standalone/index.html	21.7 kB	0 B

_{compressed-size-action}

[lex111]

Oh, that's not a good sign, it might have a negative impact on heavily customized sites.

[lex111]

@slorber if we remove ErrorBoundary from the App component, styles order will be the same as earlier. We use ErrorBoundary in Layout comp, maybe that will be enough?

[slorber]

🤷‍♂️ we still want to catch errors that may be above unfortunately, not really willing to remove it. Users might want to use a different layout or have errors thrown in <Root>

[lex111]

It's more like a rare use case, isn't it? Then I would remove the root ErrorBoundary, at least until we get consistent CSS ordering.

[slorber]

The thing is we don't know for sure why ErrorBoundary does change the CSS order, so how can we be sure that removing it will actually keep the correct order forever?

[lex111]

We already know that CSS ordering has changed as a result of this root error boundary, why this is the case is not yet known. However, better to eliminate this side effect before making new release.

[slorber]

Ok

Maybe give a try to use the theme-fallback Error in this case instead of the real theme Error? That would be better than plainly removing it

[lex111]

Sorry, but I don't know what you mean. What does theme-fallback have to do with it?

BTW, just noticed that margin-top to blog item footer is no longer applied, it was happened after merge that PR, introducing ErrorBoundary:

[Josh-Cena]

This is quite unexpected, but makes sense.

When Docusaurus mounts the <App> to the #__docusaurus element, the very first component it sees is <ErrorBoundary>. The dependency then goes like <ErrorBoundary> => @theme/Error => @theme/Layout. .docusaurus-mt-lg is defined in the styles.css of the layout, and gets loaded. At the same time the client modules are loaded via client-lifecycles-dispatcher, but this is currently put last, and always happens after loading the App.

Before this, we never loaded @theme/Layout in core, so that CSS was loaded after the Infima stylesheet. I suspect #3678 is caused by the same CSS loading setup issue...

I've added a console.log() in theme-classic/Layout and client-lifecycles-dispatchers. See that the former is loaded before the latter:

So indeed, we are not actually respecting our spec that "client modules are loaded before React starts rendering"

[Josh-Cena]

@lex111 @slorber See #5987

[slorber]

For now, I'd prefer if we reverted to the old behavior until we have a proper plan to fix these CSS ordering issues once for all.
See my comment here #5987 (comment)

To restore the old behavior, maybe we can use this in App? import Error from '@theme-init/Error';

[Josh-Cena]

To restore the old behavior, maybe we can use this in App? import Error from '@theme-init/Error';

Well, it's more about importing the original Layout, because it's Layout that has side-effects (by using a global stylesheet). Also there's #5983 that fixes the theme-init alias that needs to be merged first

[slorber]

Agree, not a good fix.
So the safest temporary solution is probably to simply remove the App error boundary for now as @lex111 suggested, so that this does not block us to release (unless you have a better proposal).

I really don't want to do any CSS ordering changes in a hurry without a careful plan

[Josh-Cena]

to simply remove the App error boundary for now

Sure, seems like a good enough fix for now if we are to release soon

don't want to do any CSS ordering changes

I can assert that there's no ordering change in #5983 (which currently just makes client modules do what they used to do), but I understand your concern. I would do the actual reordering once and for all in that PR