feat(v2): add support another Prism theme for dark mode #1984

lex111 · 2019-11-13T10:37:20Z

Motivation

I think it's worth considering the introduction of a separate Prism theme for dark mode.

This is proof of concept (I'm not sure about peft of this solution) and there is a issue with how to switch the Prism theme when changing dark mode toggle in header. Any ideas?

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Config:

//...

  themeConfig: {
    prism: {
      darkTheme: require('prism-react-renderer/themes/dracula'),
    },

//...

docusaurus-bot · 2019-11-13T10:41:40Z

Deploy preview for docusaurus-2 ready!

Built with commit 1cc3244

https://deploy-preview-1984--docusaurus-2.netlify.com

docusaurus-bot · 2019-11-13T10:42:52Z

Deploy preview for docusaurus-preview ready!

Built with commit d694a29

https://deploy-preview-1984--docusaurus-preview.netlify.com

endiliey · 2019-11-13T11:27:35Z

d there is a issue with how to switch the Prism theme when changing dark mode toggle in header. Any ideas?

maybe our hooks need to be fixed to make it more flexible.Maybe we should have something like mutation observer to listen to dom changes ? hmm. Not really sure

lex111 · 2019-11-14T14:08:33Z

@endiliey I added the simplest event bus, now when you change the theme in the navbar, the Prism theme also changes dynamically. Can you take a look the implementation please?

endiliey · 2019-11-14T15:38:31Z

Maybe we can try with dogfooding it in netlify preview first, so that others could test it too.
You can disable the theme-live-codeblock temporarily, then set the required config, and so on. So we can test it netlify preview. I'm not really sure myself on the best approach to do this hehe

cc @yangshun and @wgao19

wgao19 · 2019-11-14T15:41:18Z

how often will we use the event bus? unless we will pick it up in multiple places i feel it's a bit overkill. i think the observer hook may suffice

lex111 · 2019-11-14T16:13:23Z

Maybe we can try with dogfooding it in netlify preview first, so that others could test it too.

Makes sense, done ✔️

Perhaps in the future the event bus will be needed more, I can’t know for sure, if there is an alternative to the current solution, I only got the event bus in my mind.

i think the observer hook may suffice

Сan detail what you mean by that?

UPD: If you mean MutationObserver, then I'm not sure if this is a good solution, largely because then we rely on third-party dependency. Event bus is more transparent and understandable, IMO.

packages/docusaurus/src/client/exports/useEventBus.js

packages/docusaurus-theme-live-codeblock/src/theme/CodeBlock/index.js

wgao19 · 2019-11-20T10:52:53Z

MutationObserver is web API, no extra dependency needed.

I'm not against event bus just wondering if this is the only use case.

endiliey · 2019-11-25T09:38:05Z

The downside of using this subscription method is that the theme selector has to dispatch when we change the theme. But generally that's okay because only 1 toggle though.

mutationobserver advantage is that it can just listen to data-theme DOM and is somewhat look less complex. Less line of codes ??

some random pseudocode that i made (might not work :

// CodeBlock.js
useEffect(() => {
    const observer = new MutationObserver(mutationsList => {
      for (let mutation of mutationsList) {
        if (mutation.attributeName !== 'data-theme') {
          continue;
        }
        const { theme } = document.documentElement.dataset;
        changePrismTheme(theme);
        return;
      }
    });
    observer.observe(document.documentElement, { attributes: true });
    return () => observer.disconnect();
  }, []);

lex111 · 2019-11-25T09:45:33Z

@endiliey thank you, this is a working solution, although the option with the event bus seems to me more understandable, then should I remove it and replace it with the MutationObserver?

endiliey · 2019-11-25T09:57:47Z

Hmm im not sure. Both approach works well for me.

I just tried it locally like this,

// CodeBlock.js
  const [theme] = useTheme();
  const lightThemePrism = prism.theme || defaultTheme;
  const darkThemePrism = prism.darkTheme || defaultTheme;
  const [prismTheme, setPrismTheme] = useState(
    theme === 'dark' ? darkThemePrism : lightThemePrism,
  );

  useEffect(() => {
    const observer = new MutationObserver(mutationsList => {
      for (let mutation of mutationsList) {
        if (mutation.attributeName !== 'data-theme') {
          continue;
        }
        const {theme} = document.documentElement.dataset;

        setPrismTheme(theme === 'dark' ? darkThemePrism : lightThemePrism);
        return;
      }
    });
    observer.observe(document.documentElement, {attributes: true});

    return () => observer.disconnect();
  }, []);

And i was able to delete all the eventBus code & only CodeBlock.js need some code changes. It's much lesser code for now

endiliey · 2019-11-25T09:59:02Z

cc @yangshun and @wgao19

I'm ok with any approach 😉

endiliey

just gonna stamp. final call from other maintainers

endiliey · 2019-11-28T02:42:58Z

I’d like to suggest not to merge this yet for now. Upon working on the no flash problem, its known that “document data-theme” should be the main source of truth of whether dark mode is on or not.

This one requires toggle to dispatch an event, vanilla javascript cant dispatch a react event. Lets put this on hold till we solve the no flash problem (at least) and make sure it can work as well

wgao19 · 2019-11-28T09:06:41Z

packages/docusaurus-theme-classic/src/theme/hooks/useEventBus.js

+};
+
+const useEventBus = (type, listener, deps = []) => {
+  useEffect(() => subscribe(type, listener), deps);


maybe we can return unsubscribe method here for better cleanup?

useEffect(() => { subscribe(type, listener); return () => subscribers[type] = subscribers[type].filter(fn => fn !== listener); }, deps);

yangshun

I might be missing something, but I think the event bus is not necessary, please refer to inline comments.

Not really in favor of event bus/emitter-style of notifications. It feels like we are back to the pre-React days where we have messy event handlers sprinkled throughout the app.

In the worst case, how about we add the theme into the context and listen to the props?

yangshun · 2019-11-30T21:24:52Z

packages/docusaurus-theme-classic/src/theme/Navbar/index.js

+    e => {
+      const newTheme = e.target.checked ? 'dark' : '';
+      setTheme(newTheme);
+      dispatch('docusaurus-change-theme', {newTheme});


Why is this dispatch() even needed? The useTheme hook stores the theme and when it gets updated, them components which are using the value will also re-render. setTheme will already trigger an update for those components using useTheme. I think this is unnecessary and as a result, we probably don't need the event bus.

The useTheme hook stores the theme and when it gets updated, them components which are using the value will also re-render.

I tried go that way, but this method does not work for me.

I'll patch and try it. Give me a few days, I'm moving countries today 😅

Yes please! Although it seems to me that this is not such a bad decision, but with hooks/context it not worked out for me ;(

@yangshun any news?

The useTheme hook stores the theme and when it gets updated, them components which are using the value will also re-render.

I was completely wrong (wonky memory). useTheme is state and local to a component. setTheme only updates the theme value within the component.

yangshun · 2019-12-18T14:48:58Z

Sorry I haven't got to it. Gimme a while more. You might want to resolve the merge conflict first anyway.

yangshun

I finally got the chance to take a good look at this. I apologize for the following:

For taking so long to get back to you. Work has been really hectic and I'm busy getting my last minute #impacc for the half.
My earlier comment that theme would automatically update was incorrect. useTheme would not sync state across components - I completely forgot that useTheme uses useState, and component state is local. I found out that by doing useTheme in so many components in our code, we were initializing theme state around 7 times on certain pages (once for each codeblock). That's completely unnecessary and unintended. The proper way was for theme to be part of context. Heck, even the React context examples used theme as their example.

I'm not in favor of the event bus as a communication mechanism because the theme value is clearly part of state and we should use an idiomatic React approach.

I considered a few approaches, including listening to the window.localStorage changes and syncing the theme state across components, but the window.addEventListener('storage', ...) API doesn't allow listening for events on the same page. Hence I settled on creating a theme context that can be used by every component which needs the theme.

The good:

Individual components now have control over the theme and can set the theme and update all places which are relying on that theme context/state.
No duplicated theme state across components. It's all the way at the top of the component tree in the theme context provider

The bad:

I had to inject the theme provider in <Layout/>. People who swizzled layout will not get the updates or their code might even break. Thankfully it's just a one-line change on their part and they can easily move to this theme context approach.
Users who create their own layout need to explicitly use the theme provider as well for theming functionality, whereas the event bus approach doesn't have to, as there is no central state (distributed across the components which use it).

I'm unable to push to your PR branch, so I made a new one on this repo and the changes can be found here. Let me know your thoughts!

lex111 · 2019-12-21T08:13:55Z

@yangshun thanks for the detailed analysis! I fully support these changes, especially since you increased performance with context? Indeed, it is much better and more understandable!
You only need to explicitly specify in release notes about manually adding theme context support to Layout if one of the users has swizzled this component.
In this case, I close this PR, will you open a new one?

yangshun · 2019-12-21T08:22:01Z

You did the bulk of the work and I was only improving on your work. I think it would be ok for you to patch the commit on your existing branch and retain this PR for the discussion. Alternatively, you can create a new PR using my branch. I prefer that the feature to be attributed to you 😄

lex111 · 2019-12-21T08:26:52Z

@yangshun fine, I'll take care of the rest and complete PR :)

P.S. If you took the time to research a long-standing issue with code block, it would be very nice! 😃 (just friendly reminder, because it still bothers me)

popuguytheparrot · 2019-12-25T11:37:34Z

Hi. When it will merged?

yangshun · 2019-12-25T11:55:21Z

@lex111 I can take over this and get it merged if you allow me access to the branch/repo.

lex111 · 2019-12-25T12:27:47Z

@yangshun you got it. Sorry that I could not do it myself quickly.

yangshun · 2019-12-25T14:53:21Z

Finally merged this! I'll add docs and update our templates with this new feature in a follow-up PR.

endiliey · 2019-12-26T08:51:10Z

I think its still buggy

If you go to https://v2.docusaurus.io/docs/installation or any link, set dark mode and refresh, it will still be light theme on first page reload.

popuguytheparrot · 2019-12-26T09:11:17Z

i think need add in deps of onToggleChange theme var

yangshun · 2019-12-26T09:16:01Z

Hmm thanks for catching. I'll look into it.

yangshun · 2019-12-26T14:10:41Z

I think this only happens on the build version of the site. It works fine on development. The SSR step isn't theme-aware and always renders the light theme syntax highlighting to static HTML. The build step doesn't have access to the theme localStorage so I can only set the syntax highlighting after page load.

yangshun · 2019-12-26T15:07:00Z

Root cause - React doesn't update styles that are rendered via SSR - facebook/react#11128

endiliey · 2019-12-27T14:32:31Z

yes only in prod. I explained it before on my dark mode fouc fix

See #2069

isClient context is needed because we need to do two pass rendering
Why do we need two pass rendering ?
For full reasoning, please check the whole thread starting from this comment #2057 (comment)

binarylogic · 2019-12-28T16:50:01Z

🎉 thanks for working on this, we're looking forward to this change.

lex111 requested a review from yangshun November 13, 2019 10:37

facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 13, 2019

lex111 requested a review from endiliey November 13, 2019 10:37

lex111 force-pushed the prism-dark-theme branch from 8e97f92 to cb8e98f Compare November 14, 2019 19:01

lex111 changed the title ~~feat(v2): add support another Prism theme for dark mode [WIP]~~ feat(v2): add support another Prism theme for dark mode Nov 14, 2019

endiliey reviewed Nov 16, 2019

View reviewed changes

packages/docusaurus/src/client/exports/useEventBus.js Outdated Show resolved Hide resolved

endiliey suggested changes Nov 16, 2019

View reviewed changes

packages/docusaurus-theme-live-codeblock/src/theme/CodeBlock/index.js Show resolved Hide resolved

endiliey approved these changes Nov 25, 2019

View reviewed changes

endiliey requested review from wgao19 and yangshun and removed request for yangshun November 25, 2019 16:04

endiliey mentioned this pull request Nov 27, 2019

feat(v2): injectHtmlTags API to inject head and/or body html tags #2057

Merged

wgao19 approved these changes Nov 28, 2019

View reviewed changes

wgao19 reviewed Nov 28, 2019

View reviewed changes

endiliey added status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. pr: new feature This PR adds a new API or behavior. labels Nov 28, 2019

yangshun suggested changes Nov 30, 2019

View reviewed changes

lex111 mentioned this pull request Dec 10, 2019

fix(v2): add syntax highlight to generated colors #2114

Merged

yangshun reviewed Dec 21, 2019

View reviewed changes

lex111 closed this Dec 21, 2019

lex111 reopened this Dec 21, 2019

lex111 and others added 5 commits December 25, 2019 22:28

feat(v2): add support another Prism theme for dark mode

290c215

add dynamically change Prism theme

020c70a

Prepare for review

22fe6ef

PR review changes

d7701c6

feat(v2): useThemeContext

1cc3244

yangshun force-pushed the prism-dark-theme branch from d694a29 to 1cc3244 Compare December 25, 2019 14:45

yangshun removed the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Dec 25, 2019

yangshun merged commit 92c68ed into facebook:master Dec 25, 2019

yangshun deleted the prism-dark-theme branch December 25, 2019 14:52

yangshun mentioned this pull request Dec 26, 2019

fix(v2): render correct syntax theme for current website theme #2150

Merged

lex111 mentioned this pull request Feb 28, 2021

fix(v2): evaluate code in live editor on client only #4318

Merged

feat(v2): add support another Prism theme for dark mode #1984

feat(v2): add support another Prism theme for dark mode #1984

Conversation

lex111 commented Nov 13, 2019

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

docusaurus-bot commented Nov 13, 2019 • edited Loading

docusaurus-bot commented Nov 13, 2019 • edited Loading

endiliey commented Nov 13, 2019

lex111 commented Nov 14, 2019

endiliey commented Nov 14, 2019

wgao19 commented Nov 14, 2019

lex111 commented Nov 14, 2019 • edited Loading

wgao19 commented Nov 20, 2019

endiliey commented Nov 25, 2019

lex111 commented Nov 25, 2019

endiliey commented Nov 25, 2019 • edited Loading

endiliey commented Nov 25, 2019

endiliey left a comment

Choose a reason for hiding this comment

endiliey commented Nov 28, 2019 • edited Loading

wgao19 Nov 28, 2019

Choose a reason for hiding this comment

yangshun left a comment

Choose a reason for hiding this comment

yangshun Nov 30, 2019

Choose a reason for hiding this comment

lex111 Dec 9, 2019

Choose a reason for hiding this comment

yangshun Dec 10, 2019

Choose a reason for hiding this comment

lex111 Dec 12, 2019

Choose a reason for hiding this comment

lex111 Dec 18, 2019

Choose a reason for hiding this comment

yangshun Dec 21, 2019

Choose a reason for hiding this comment

yangshun commented Dec 18, 2019

yangshun left a comment • edited Loading

Choose a reason for hiding this comment

lex111 commented Dec 21, 2019

yangshun commented Dec 21, 2019 • edited Loading

lex111 commented Dec 21, 2019

popuguytheparrot commented Dec 25, 2019

yangshun commented Dec 25, 2019

lex111 commented Dec 25, 2019

yangshun commented Dec 25, 2019

endiliey commented Dec 26, 2019 • edited Loading

popuguytheparrot commented Dec 26, 2019

yangshun commented Dec 26, 2019

yangshun commented Dec 26, 2019 • edited Loading

yangshun commented Dec 26, 2019

endiliey commented Dec 27, 2019

binarylogic commented Dec 28, 2019

docusaurus-bot commented Nov 13, 2019 •

edited

Loading

docusaurus-bot commented Nov 13, 2019 •

edited

Loading

lex111 commented Nov 14, 2019 •

edited

Loading

endiliey commented Nov 25, 2019 •

edited

Loading

endiliey commented Nov 28, 2019 •

edited

Loading

yangshun left a comment •

edited

Loading

yangshun commented Dec 21, 2019 •

edited

Loading

endiliey commented Dec 26, 2019 •

edited

Loading

yangshun commented Dec 26, 2019 •

edited

Loading