refactor(theme-{classic,common}): refactor ColorModeToggle + useColorMode() hook #6930

lex111 · 2022-03-17T08:36:09Z

Breaking change:

the props of the ColorModeToggle have changed

Deprecated:

Some previous public APIs now emit a warning, they'll be removed so please do the migration

useColorMode().isDarkTheme => useColorMode().colorMode === "dark"
useColorMode().setLightTheme => useColorMode().setColorMode("light")
useColorMode().setDarkTheme => useColorMode().setColorMode("dark")

Motivation

The button should be accessible, semantically correct, and consistent with the general style of site. Also removed some dead code, which is unnecessary given that the <button> element is used.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview.

Related PRs

Josh-Cena

Thanks!

When I implemented the button, I was largely unsure whether a button or a checkbox has better semantics. IMO a button is stateless (click,click,click) but a checkbox is stateful (checked/unchecked), so I eventually went for what we had before. But since we have aria label, I think it's actually better as a button 👍

netlify · 2022-03-17T08:43:36Z

✔️ [V2]

🔨 Explore the source changes: 053bc9c

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

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

github-actions · 2022-03-17T08:44:03Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🔴 Performance	37
🟢 Accessibility	100
🟢 Best practices	92
🟢 SEO	100
🟢 PWA	90

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

github-actions · 2022-03-17T08:48:55Z

Size Change: -726 B (0%)

Total Size: 801 kB

Filename	Size	Change
`website/build/assets/css/styles.********.css`	104 kB	-228 B (0%)
`website/build/assets/js/main.********.js`	608 kB	-529 B (0%)
`website/build/index.html`	38.8 kB	+31 B (0%)

ℹ️ View Unchanged

Filename	Size
`website/.docusaurus/globalData.json`	49.9 kB

_{compressed-size-action}

slorber

Thanks

I'm not skilled enough in a11y to tel if a button or checkbox is better so I trust you there

I'd like to refactor the props API before merging

Note I'd like to eventually allow this color mode switch to work without JS/hydration (not a very high priority though), and it might require a checkbox in the future, but it's not very important for now, just wanted to let you know ;)

https://twitter.com/sebastienlorber/status/1500872668727451651

slorber · 2022-03-17T11:12:58Z

packages/docusaurus-theme-classic/src/theme-classic.d.ts

  export interface Props {
    readonly className?: string;
-    readonly checked: boolean;
-    readonly onChange: (e: SyntheticEvent) => void;
+    readonly darkModeEnabled: boolean;


For future flexibility I'd like to avoid having the term "dark mode" in this API here

In the future we should account for the fact that other official color modes might be added to the CSS spec (as the spec explains), or that we might simply provide other color modes ourselves and let users implement custom ones with swizzling.

In fact, checked + onChange were also not very good choices either as this assumes there are only 2 values (true/false) while there might be more in the future or if the user wants more values

I'd rather have value: "light" | "dark" for example

Note that we marked this component as safe for swizzle, so it means changing its props is now considered as a breaking change.

As we are still in beta that looks like the good time to do this, as in the future this means the change will be done in a new major version

Done it, what is the better term to use: colorMode, colorScheme, or just "theme"?

slorber · 2022-03-17T11:23:13Z

Not blocking this PR but I also have this other refactor with a new NavbarColorModeToggle here: https://github.com/facebook/docusaurus/pull/6895/files#diff-51072c1a59c9c7f1396c4aaf865423fe896bf8408a9efb763e8ce832e526934a

I'll update my PR after merging yours.

Similarly I don't like the existing hook assuming color mode can only be 2 values, ie const {isDarkTheme, toggle} = useColorMode();. I'm likely to keep isDarkTheme for retrocompatibility but add a warning to deprecate it, this hook is used by a few sites already.

Josh-Cena · 2022-03-17T11:28:25Z

TBH, adding a third color mode choice would be a huge change because it means any existing UI/UX won't work. I'd rather change the API until we actually make that happen. I haven't seen any content site with more than two color modes.

slorber · 2022-03-17T11:42:30Z

I haven't seen any content site with more than two color modes.

I have seen a few sites with more than light/dark theme.

In the past, before dark theme was popular, there were so many example of sites/forums providing multiple themes too.

The CSS explains that prefers-color-scheme is likely to be extended

There are also large scale websites implementing alternative color schemes for accessibility reasons, like GitHub: https://github.blog/changelog/2021-09-29-colorblind-themes-beta/

In general, every single time I didn't apply pre-emptive pluralization (https://www.swyx.io/preemptive-pluralization) it ends up bitting me a few months/years later, so I really prefer to do this now when the pain is still low

Josh-Cena · 2022-03-17T12:01:20Z

I was carefully saying "content sites" to rule out GitHub 😄 These sites offering choices often has a centralized settings panel which is probably an overkill for a content site, especially a documentation site.

That said, I'm not against the idea of having multiple color themes. But at that time we can't use either a button or a toggle whatsoever.

Josh-Cena · 2022-03-17T13:14:17Z

packages/docusaurus-theme-common/src/utils/colorModeUtils.tsx

@@ -34,7 +34,7 @@ const themes = {
  dark: 'dark',
 } as const;

-type Themes = typeof themes[keyof typeof themes];
+export type Themes = typeof themes[keyof typeof themes];


I personally don't like the name Themes because it's a union, not a collection. Maybe rename this as ColorTheme or ColorMode for uniformity?

Agreed , I'm not really sure what to consider as the source of truth for getting current (supported) color themes, so maybe this isn't the fit place. We need to decide which term to use, I like "color mode" or "color scheme" better than "[color] theme".

Yes, since the component is called ColorModeToggle anyways, let's call this type ColorMode

Although I don't like using the "theme" term, it's better to be consistent in this case anyway. So even in the ColorModeToggle component I use "theme" term. This may not be very clear, but it is consistent with colorModeUtils for now.

Actually, I'm going to use ColorMode instead and refactor a bit the colorModeUtils file (keeping old public apis for retrocompatibility but we should plan to remove them), so it should be consistent in the long term

I think there's value to using another term as otherwise we'd have 2 concept of "themes" in Docusaurus: theme plugin vs dark/light

For now I won't touch the CSS selectors / localStorage keys though as it would be quite annoying for users

* chore: tighten ESLint config * more refactor * refactor push * fix

Co-authored-by: Joshua Chen <[email protected]> # Conflicts: # packages/docusaurus-theme-common/src/index.ts

packages/docusaurus-theme-common/src/utils/colorModeUtils.tsx

Josh-Cena · 2022-03-18T13:33:24Z

I think this is good now. Can confirm everything works, even our wrapped color mode toggle

refactor(theme-classic): color mode toggle button fixes

6a5f1d5

lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Mar 17, 2022

lex111 requested review from slorber and Josh-Cena as code owners March 17, 2022 08:36

facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 17, 2022

lex111 mentioned this pull request Mar 17, 2022

RFC: default UI polishing #6717

Open

2 tasks

Josh-Cena approved these changes Mar 17, 2022

View reviewed changes

slorber requested changes Mar 17, 2022

View reviewed changes

refactor API: get rid of "dark mode" term

f691633

Josh-Cena reviewed Mar 17, 2022

View reviewed changes

Use theme instead of colorMode

788da9d

lex111 force-pushed the lex111/color-mode-toggle-btn-fix branch from ec6aad2 to 788da9d Compare March 17, 2022 17:28

Josh-Cena and others added 8 commits March 18, 2022 12:16

chore: tighten ESLint config (#6931)

b19cab5

* chore: tighten ESLint config * more refactor * refactor push * fix

refactor(theme-classic): little breadcrumbs improvements (#6932)

c3a8afe

feat(core,theme): useRouteContext + HtmlClassNameProvider (#6933)

6b9726b

Co-authored-by: Joshua Chen <[email protected]> # Conflicts: # packages/docusaurus-theme-common/src/index.ts

refactor color mode

c7370e2

Merge branch 'main' into lex111/color-mode-toggle-btn-fix

ecf901b

minor change

1c414a0

Update index.ts

1e53895

Update index.ts

8b56207

Josh-Cena reviewed Mar 18, 2022

View reviewed changes

packages/docusaurus-theme-common/src/utils/colorModeUtils.tsx Show resolved Hide resolved

slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Mar 18, 2022

refactor

2ea1963

Josh-Cena added 2 commits March 18, 2022 20:53

fix

ed1aa2f

refactor

7f7e97e

fix last usages of deprecated useColorMode apis

053bc9c

slorber changed the title ~~refactor(theme-classic): color mode toggle button fixes~~ refactor(theme-classic): refactor ColorModeToggle + useColorMode() hook Mar 18, 2022

Josh-Cena changed the title ~~refactor(theme-classic): refactor ColorModeToggle + useColorMode() hook~~ refactor(theme-{classic,common}): refactor ColorModeToggle + useColorMode() hook Mar 18, 2022

slorber merged commit ecbe0b2 into main Mar 18, 2022

slorber deleted the lex111/color-mode-toggle-btn-fix branch March 18, 2022 14:28

erickzhao mentioned this pull request Jul 27, 2022

refactor: stop using deprecated isDarkTheme API electron/website#237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(theme-{classic,common}): refactor ColorModeToggle + useColorMode() hook #6930

refactor(theme-{classic,common}): refactor ColorModeToggle + useColorMode() hook #6930

lex111 commented Mar 17, 2022 •

edited by slorber

Loading

Josh-Cena left a comment

netlify bot commented Mar 17, 2022 •

edited

Loading

github-actions bot commented Mar 17, 2022 •

edited

Loading

github-actions bot commented Mar 17, 2022 •

edited

Loading

slorber left a comment

slorber Mar 17, 2022

slorber Mar 17, 2022

lex111 Mar 17, 2022

slorber commented Mar 17, 2022 •

edited

Loading

Josh-Cena commented Mar 17, 2022

slorber commented Mar 17, 2022

Josh-Cena commented Mar 17, 2022

Josh-Cena Mar 17, 2022

lex111 Mar 17, 2022

Josh-Cena Mar 17, 2022

lex111 Mar 17, 2022

slorber Mar 18, 2022 •

edited

Loading

Josh-Cena commented Mar 18, 2022

refactor(theme-{classic,common}): refactor ColorModeToggle + useColorMode() hook #6930

refactor(theme-{classic,common}): refactor ColorModeToggle + useColorMode() hook #6930

Conversation

lex111 commented Mar 17, 2022 • edited by slorber Loading

Breaking change:

Deprecated:

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

Josh-Cena left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 17, 2022 • edited Loading

github-actions bot commented Mar 17, 2022 • edited Loading

github-actions bot commented Mar 17, 2022 • edited Loading

slorber left a comment

Choose a reason for hiding this comment

slorber Mar 17, 2022

Choose a reason for hiding this comment

slorber Mar 17, 2022

Choose a reason for hiding this comment

lex111 Mar 17, 2022

Choose a reason for hiding this comment

slorber commented Mar 17, 2022 • edited Loading

Josh-Cena commented Mar 17, 2022

slorber commented Mar 17, 2022

Josh-Cena commented Mar 17, 2022

Josh-Cena Mar 17, 2022

Choose a reason for hiding this comment

lex111 Mar 17, 2022

Choose a reason for hiding this comment

Josh-Cena Mar 17, 2022

Choose a reason for hiding this comment

lex111 Mar 17, 2022

Choose a reason for hiding this comment

slorber Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Josh-Cena commented Mar 18, 2022

lex111 commented Mar 17, 2022 •

edited by slorber

Loading

netlify bot commented Mar 17, 2022 •

edited

Loading

github-actions bot commented Mar 17, 2022 •

edited

Loading

github-actions bot commented Mar 17, 2022 •

edited

Loading

slorber commented Mar 17, 2022 •

edited

Loading

slorber Mar 18, 2022 •

edited

Loading