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

refactor: control base styling of code blocks via CSS vars #7172

Merged
merged 3 commits into from
Apr 14, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Apr 14, 2022

Motivation

My recent PR #7007 broke applying proper background of Prism theme to code blocks (it is especially noticeable when dark mode is on). Unfortunately, inheritance when used with CSS vars works unexpected from it way. Also, as I mentioned earlier https://github.com/facebook/docusaurus/pull/7036/files/ebb2d9e303eeb5978ffcf6573b0162b27868ba43#r845543297 to group the code buttons would require adding style attributes coming from Prism on each button.
Overall, the code complexity increases and with it, increasingly difficult to ensure proper styling of block codes with all use cases in mind.
So I propose to dynamically create two CSS vars corresponding to the current Prism color theme. This way we can avoid overusing of style attribute on many elements and it is just a predictable way to control styling as opposed to many inherited values.

Ideally I would of course like to do away with inline styles completely when highlighting the code itself, maybe in the future it's worth thinking about how to do this (dynamically create CSS classes like token-line etc.).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Compare https://docusaurus.io/tests/pages/code-block-tests and respective page on preview site.

Related PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 14, 2022
@netlify
Copy link

netlify bot commented Apr 14, 2022

[V2]

Name Link
🔨 Latest commit e0076d1
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/625844603789ae0008d2420c
😎 Deploy Preview https://deploy-preview-7172--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Apr 14, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 53
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

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

@github-actions
Copy link

github-actions bot commented Apr 14, 2022

Size Change: +98 B (0%)

Total Size: 798 kB

Filename Size Change
website/build/assets/css/styles.********.css 106 kB +98 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 50 kB
website/build/assets/js/main.********.js 604 kB
website/build/index.html 38.6 kB

compressed-size-action

@slorber
Copy link
Collaborator

slorber commented Apr 14, 2022

My recent PR #7007 broke applying proper background of Prism theme to code blocks (it is especially noticeable when dark mode is on).

I see absolutely 0 change between prod and preview on the test page 🤷‍♂️ are you sure anything is broken and this fixes anything?

Not against applying CSS vars at the top like that but keep in mind that anything applied in useEffect is run after React hydration and is likely to produce FOUC.

We now have a different kind of FOUC on code blocks compared to before:

This is what I see when I throttle to Fast 3G before React hydration:

CleanShot 2022-04-14 at 15 39 01@2x

Before the light theme was applied, and then dark theme => this was better before IMHO, now the progressive enhancement experience is less controllable

@lex111
Copy link
Contributor Author

lex111 commented Apr 14, 2022

The background is different now (i.e background of the code content should be darker, same like the code title):

image

This is what I see when I throttle to Fast 3G before React hydration:

Well I see currently roughly the same on test production page: https://docusaurus.io/tests/pages/code-block-tests

So we need to define the CSS vars in on building step, right?

@slorber
Copy link
Collaborator

slorber commented Apr 14, 2022

The background is different now (i.e background of the code content should be darker, same like the code title):

I'm comparing side by side in dark mode:

Looks like there's a little change, can't really see 🤪 it but devtools confirm 👍


So we need to define the CSS vars in on building step, right?

Not sure what's the best solution here but we likely need to move prism theme colors to CSS if we want to avoid any FOUC

:root {
  ...
}

html[data-theme='dark'] {
 ...
}

Maybe we need to create a prism-className-theme or something 🤷‍♂️ This is a bit out of scope of current PR but we can look on how to improve all this.

Will do some minor tweaks and merge current PR

@lex111
Copy link
Contributor Author

lex111 commented Apr 14, 2022

Do you mean generating a separate CSS file for each of used Prism themes - light and dark, and then include the proper one of them depending on the current color mode on page?

Btw, do we need to additionally define new CSS vars related with Prism in the FOUC prevention function https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-classic/src/index.ts#L27?

@Josh-Cena
Copy link
Collaborator

Yes, that's one of my major problems with Prism: poor SSR support. Shiki handles this by rendering two blocks on server-side and hiding one with CSS. Doesn't play well without JS, though, but better than Prism behavior for 99% of the users IMO.

@slorber
Copy link
Collaborator

slorber commented Apr 14, 2022

Yes, that's one of my major problems with Prism: poor SSR support. Shiki handles this by rendering two blocks on server-side and hiding one with CSS. Doesn't play well without JS, though, but better than Prism behavior for 99% of the users IMO.

This is what we do for ThemedImage too, but IMHO it's a bit heavy particularly for code blocks to output them twice in HTML files and can also increase HTML size significantly. Not sure it's our best option in this case although it would be simple to setup.

Btw, do we need to additionally define new CSS vars related with Prism in the FOUC prevention function

We should either have all colors in light theme, or all colors in dark theme (including tokens). Just preventing the background FOUC would lead to inconsistent / inaccessible result like light tokens on light background, probably worst than FOUC.

@slorber
Copy link
Collaborator

slorber commented Apr 14, 2022

I moved CSS variables from 1 top root element to multiple local elements for a few reasons:

  • simpler code, not split across many files
  • easier to ensure that the code block theme and CSS variables stay in sync
  • CSS vars are rendered on the server too => revert to previous FOUC behavior (light -> dark)
  • Gives more flexibility for later (for example if we want to try to render the code block in both light/dark to prevent any FOUC)

@slorber slorber merged commit ad1526a into main Apr 14, 2022
@slorber slorber deleted the lex111/prism-code-block branch April 14, 2022 16:16
@slorber slorber added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Apr 14, 2022
@lex111
Copy link
Contributor Author

lex111 commented Apr 14, 2022

That's not really what I wanted originally, since it is not much different from the previous approach. Currently we have the same code duplication, just a lot of the same definition of CSS variables. Can't we define CSS vars only once, for example in root html element or in style element in head, and avoid FOUC issue at same time?

@slorber
Copy link
Collaborator

slorber commented Apr 20, 2022

For now I'd prefer to keep this duplication on purpose, as I'd like to try to see the impact of rendering twice the same code block and avoid 100% of the FOUC.

Moving those vars at the top is technically possible but is just a micro optimization that split the code and makes things less flexible

@lex111
Copy link
Contributor Author

lex111 commented Apr 20, 2022

I would call it a better approach than micro-optimization. As I mentioned above, it would be ideal not to use any inline styles at all. HTML markup of code blocks is much cleaner, not to mention the fact that there were no its re-renders when switching color mode.

@slorber
Copy link
Collaborator

slorber commented Apr 20, 2022

I would call it a better approach than micro-optimization. As I mentioned above, it would be ideal not to use any inline styles at all.

It's another approach that removes flexibility (ie we can't render a dark and light code block on the same page) and only reduces a little bit of markup.

Using inline style is fine occasionally, many animation libs use this for example. As long as it's used sparingly it's ok.

not to mention the fact that there were no its re-renders when switching color mode.

We already have to re-render the code-block anyway, because it's already using inline styles for each line tokens.

Rendering and using inline styles on the container does not have a significant impact

@lex111
Copy link
Contributor Author

lex111 commented Apr 20, 2022

It's another approach that removes flexibility (ie we can't render a dark and light code block on the same page) and only reduces a little bit of markup.

Instead it would be switching on the desired CSS file (like prism-light.css or prism-dark.css), or setting needed styles in the style tag, but only once in head tag.

Especially applying the style attribute still does not completely eliminate the issue with FOUC. Alright, although I would at least somehow remove the definition of Prism CSS vars for each code block instance.

ezgif com-gif-maker

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: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants