feat(v2): add ThemedImage component by Simek · Pull Request #3730 · facebook/docusaurus

Simek · 2020-11-12T10:59:21Z

Motivation

For some websites it is important to be able to switch the image source based on the current theme. This PR introduces the simple theme helper component - ThemedImage - which allow users to specify the lightSrc and darkSrc props and switches the image content dynamically on theme change.

The docs has also been updated within this PR.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Changes has been tested locally using Docusuaurus V2 website.

Preview

Related PRs

None.

netlify · 2020-11-12T11:14:08Z

Deploy preview for docusaurus-2 ready!

Built with commit 80309ca

https://deploy-preview-3730--docusaurus-2.netlify.app

slorber · 2020-11-12T11:15:19Z

packages/docusaurus-theme-classic/src/theme/ThemedImage/index.tsx

+  }
+}
+
+export default ThemedImage;


This component will only render one image (the light one) on the other on server-rendered markup.

See useTheme hook:

const getInitialTheme = () => { if (!ExecutionEnvironment.canUseDOM) { return themes.light; // SSR: we don't care } return coerceToTheme(document.documentElement.getAttribute('data-theme')); };

On hydration, the light image will be replaced with the dark one, producing an unwanted flash.

The only viable solution to fix the problem is to always render the 2 images, and display one or the other thanks to CSS (which is loaded before the user actually see anything, so there's no flash)

img.themed-image{ display: none; } html[data-theme='light'] img.themed-image.themed-image--light { display: block; } html[data-theme='dark'] img.themed-image.themed-image--dark { display: block; }

Note: it's worth considering that light/dark is common nowadays but the CSS spec says this might be expanded to more values in the future. Having an API surface that is more extensible could be more future-proof (ie user can declare its own theme name, we should rather have sources={dark: darkSrc,light:lightSrc,sepia: sepiaSrc})

slorber

I think it would produce an hydration flash.

Can you add an example somewhere so that we can see it in action in deploy previews? (there an example md page if you want)

Simek · 2020-11-12T11:23:10Z

I think it would produce an hydration flash.

Can you add an example somewhere so that we can see it in action in deploy previews? (there an example md page if you want)

You can find the example in the added section on the Markdown Features page.

slorber · 2020-11-12T11:27:51Z

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

+    readonly darkSrc?: string;
+  } & ComponentProps<'img'>;
+
+  const ThemedImage: () => JSX.Element;


shouldn't you use the props here?

const ThemedImage: (props: Props) => JSX.Element;

This has been taken from the Tabs component types already in that file (https://github.com/facebook/docusaurus/blob/master/packages/docusaurus-theme-classic/src/types.d.ts#L395):

const Tabs: () => JSX.Element; export default Tabs;

TS should be happy with that construction because this method only serves as the definition for the returned value.

slorber · 2020-11-12T11:28:52Z

Ah yeah, didn't see, but unfortunately Netlify is failing so can't check the preview

12:14:04 PM: Module not found: Can't resolve '@theme/ThemedImage' in '/opt/build/repo/website/docs'
Client bundle compiled with errors therefore further build is impossible.

Simek · 2020-11-12T11:34:30Z

Ah yeah, didn't see, but unfortunately Netlify is failing so can't check the preview

12:14:04 PM: Module not found: Can't resolve '@theme/ThemedImage' in '/opt/build/repo/website/docs'
Client bundle compiled with errors therefore further build is impossible.

@slorber I saw the log, working on this right now. But it is annoying that during the development the app runs correctly and do not fail or warn user in any way, but on build is failing due to some ambiguous errors.

Is there any way to improve this processes and make the results of both modes (dev & build/serve) consistent?

Simek · 2020-11-12T12:13:10Z

Currently CI fail is related to the issue with V1 build (probably caused by mentioned above PR):

12:48:41 PM: Docusaurus v1 running as a Netlify deploy preview
12:48:41 PM: Netlify deploy previews will only deploy a subset of available versions: 1.14.6 - 1.9.x
12:48:42 PM: $ BASE_URL=/v1/ yarn build:v1
12:48:42 PM: $ yarn workspace docusaurus-1-website build
12:48:43 PM: $ docusaurus-build
12:48:44 PM: internal/modules/cjs/loader.js:583
12:48:44 PM:     throw err;
12:48:44 PM:     ^
12:48:44 PM: Error: Cannot find module '@babel/helper-skip-transparent-expression-wrappers'
12:48:44 PM:     at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
12:48:44 PM:     at Function.Module._load (internal/modules/cjs/loader.js:507:25)
12:48:44 PM:     at Module.require (internal/modules/cjs/loader.js:637:17)
12:48:44 PM:     at require (internal/modules/cjs/helpers.js:22:18)
12:48:44 PM:     at Object.<anonymous> (/opt/build/repo/node_modules/@babel/preset-env/node_modules/@babel/plugin-transform-spread/lib/index.js:10:48)
12:48:44 PM:     at Module._compile (internal/modules/cjs/loader.js:689:30)
12:48:44 PM:     at Module._compile (/opt/build/repo/node_modules/pirates/lib/index.js:99:24)
12:48:44 PM:     at Module._extensions..js (internal/modules/cjs/loader.js:700:10)
12:48:44 PM:     at Object.newLoader [as .js] (/opt/build/repo/node_modules/pirates/lib/index.js:104:7)
12:48:44 PM:     at Module.load (internal/modules/cjs/loader.js:599:32)
12:48:44 PM: error Command failed with exit code 1.
12:48:44 PM: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
12:48:44 PM: error Command failed.
12:48:44 PM: Exit code: 1

lex111 · 2020-11-12T12:26:01Z

packages/docusaurus-theme-classic/src/theme/ThemedImage/index.tsx

+  const {isDarkTheme} = useThemeContext();
+  const {src, darkSrc, lightSrc, alt = '', ...propsRest} = props;
+
+  if (darkSrc && isDarkTheme) {


Let's not duplicate essentially the same code, let's use a ternary expression to determine the desired image src?

And why is there a prop just src? In this case need use native <img> element, not ThemedImage.

Let's not duplicate essentially the same code, let's use a ternary expression to determine the desired image src?

I choose the standard conditional structure for the redability.

And why is there a prop just src? In this case need use native <img> element, not ThemedImage.

Because of TS types inheritance - & ComponentProps<'img'>; - and props passing. It also serves as fallback, if user provide src instead of lightSrc to the themed image.

lex111 · 2020-11-12T12:27:52Z

Currently CI fail is related to the issue with V1 build (probably caused by mentioned above PR):

Most likely the reason for this is Netlify cache, because the tests in the PR with the update deps were successful.

Simek · 2020-11-12T12:32:33Z

Currently CI fail is related to the issue with V1 build (probably caused by mentioned above PR):

Most likely the reason for this is Netlify cache, because the tests in the PR with the update deps were successful.

Hmm, interesting. Can you re-run the pipeline than?

lex111 · 2020-11-12T12:35:17Z

Maybe @slorber can do it, but I don't have access for that :(

Simek · 2020-11-12T12:52:11Z

@lex111 I have pushed the ternary refactor commit but the CI still fails on the same V1 error. Are you sure it is only the Netlify cache issue?

slorber · 2020-11-12T13:17:03Z

just relaunched it

slorber · 2020-11-12T13:21:34Z

packages/docusaurus-theme-bootstrap/src/types.d.ts

+    readonly darkSrc?: string;
+  } & ComponentProps<'img'>;
+
+  const ThemedImage: () => JSX.Element;


despite other examples not declaring components props properly, I think we should do this (cf TabItem, Layout).

We'll make sure all comps have props: Props over time

I'm not against this change, but this sound like a feature request or a bug fix request, so I think you should handle this for all theme components in the separate PR.

lex111 · 2020-11-12T13:24:44Z

website/docs/markdown-features.mdx

+
+<ThemedImage
+  alt="Docusaurus themed image"
+  lightSrc={useBaseUrl('img/docusaurus_keytar.svg')}


Why don't we just pass the path as regular string? Accordingly, inside the ThemedImage component, apply useBaseUrl to all src paths? Then users would not import useBaseUrl in their MDX doc.

@lex111 totally agree with that. BTW I imagined we could even have a Docusaurus image component just for this usecase

I'm not sure if processing the base URL inside the component is a good idea. It can blur the functionality and cause a confusion (why img need useBaseURL when ThemeImage do not need it)? The goal was to create simple helper not the robust, alternative image component.

And again, I have just looked at the way img tag is used on the Docusuaurus V2 website - https://github.com/facebook/docusaurus/blob/master/website/src/pages/index.js#L77.

My long-term goal is to simply deprecate useBaseUrl(). But this can probably be done later, so if we don't apply it in the component today, it's fine

lex111 · 2020-11-12T13:32:13Z

@Simek sorry, CI failed again due to the Bootstrap theme. @slorber I think we need to remove it from the CI process, because in fact it is an unmaintained theme, which creates a lot of difficulties.

packages/docusaurus-theme-bootstrap/src/theme/ThemedImage/index.tsx

slorber · 2020-11-12T13:34:41Z

agree, you can remove the bootstrap theme from the netlify scripts and the netlify HTML page. We'll add it back when it's more production ready

Simek · 2020-11-12T13:52:44Z

@slorber @lex111 It looks like useThemeContext stub is working but the CI fails back at the V1 build issue.

Simek · 2020-11-12T14:40:42Z

It looks like the local rebase and force push of PR branch fixed the V1 CI issue.

slorber · 2020-11-12T15:45:25Z

@Simek I added an example so that you understand why your js-based solution will not work in all cases:

<ThemedImage
  alt="Docusaurus themed image"
  lightSrc={
    'https://images.pexels.com/photos/4734723/pexels-photo-4734723.jpeg?auto=compress&cs=tinysrgb&dpr=2&w=200'
  }
  darkSrc={
    'https://images.pexels.com/photos/5828718/pexels-photo-5828718.jpeg?auto=compress&cs=tinysrgb&dpr=2&w=200'
  }
/>

Try refreshing this page, with dark mode, and caching enabled: https://deploy-preview-3730--docusaurus-2.netlify.app/classic/docs/markdown-features/

Here, the images added are cached aggressively. The image being served by the server will appear on the user screen before React has time to hydrate.
As the SSR theme is hardcoded, the user will always see the light image first. If he has the black theme, after hydration only, the light image will be switched to the dark image, which produces a flickering experience.

You didn't see this issue because:

your images are not cached, and appear after React hydration
your images are not at the top, and the scroll to anchor link is slightly delayed

I think the server should always render both images, but only display one thanks to CSS.
Once React has hydrated (useDocusaurusContext().isClient=true), we can only add a single image in the DOM to avoid useless resource consumption.

Simek · 2020-11-13T10:49:15Z

@slorber PR has been updated, mostly according to the review suggestions. Feel free to improve or change the docs section (I'm way better at improving the docs rather than creating the initial version).

Please remember to remove the SSR example after the successful review, but before the merge. 🙂

Simek requested review from lex111 and slorber as code owners November 12, 2020 10:59

facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 12, 2020

slorber reviewed Nov 12, 2020

View reviewed changes

slorber requested changes Nov 12, 2020

View reviewed changes

slorber reviewed Nov 12, 2020

View reviewed changes

Simek mentioned this pull request Nov 12, 2020

chore(v2): upgrade dependencies #3727

Merged

lex111 reviewed Nov 12, 2020

View reviewed changes

slorber reviewed Nov 12, 2020

View reviewed changes

lex111 reviewed Nov 12, 2020

View reviewed changes

slorber reviewed Nov 12, 2020

View reviewed changes

packages/docusaurus-theme-bootstrap/src/theme/ThemedImage/index.tsx Show resolved Hide resolved

feat(v2): add ThemedImage component

82475a3

Simek force-pushed the add-themed-image-component branch from b383945 to 82475a3 Compare November 12, 2020 14:31

add themed image problematic example

e23b4d4

refactor, SSR fix, openness about extending img tag, docs update

4a317e8

refactor themed-image

05f2da8

slorber added the pr: new feature This PR adds a new API or behavior. label Nov 13, 2020

slorber and others added 2 commits November 13, 2020 14:13

update themed image doc

db09dd5

Merge branch 'master' into add-themed-image-component

80309ca

slorber merged commit 9d90e89 into facebook:master Nov 13, 2020

Simek mentioned this pull request Nov 13, 2020

refactor(v2): introduce Logo component, remove useLogo hook #3745

Merged

This was referenced Nov 17, 2020

Pages don't pick up HTML files in 2.0 #2833

Closed

useLogo hook does not update on theme change #2646

Closed

lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020

Uh oh!

Conversation

Simek commented Nov 12, 2020

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Preview

Related PRs

Uh oh!

netlify bot commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slorber left a comment

Choose a reason for hiding this comment

Uh oh!

Simek commented Nov 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slorber commented Nov 12, 2020

Uh oh!

Simek commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simek commented Nov 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lex111 commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simek commented Nov 12, 2020

Uh oh!

lex111 commented Nov 12, 2020

Uh oh!

Simek commented Nov 12, 2020

Uh oh!

slorber commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simek Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lex111 commented Nov 12, 2020

Uh oh!

Uh oh!

slorber commented Nov 12, 2020

Uh oh!

Simek commented Nov 12, 2020

Uh oh!

Simek commented Nov 12, 2020

Uh oh!

slorber commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simek commented Nov 13, 2020

Uh oh!

Reviewers

Assignees

netlify bot commented Nov 12, 2020 •

edited

Loading

Simek commented Nov 12, 2020 •

edited

Loading

lex111 commented Nov 12, 2020 •

edited

Loading

slorber commented Nov 12, 2020 •

edited

Loading

Simek Nov 12, 2020 •

edited

Loading

slorber commented Nov 12, 2020 •

edited

Loading