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

chore: add light/dark mode theme detection to image component example #53760

Conversation

styfle
Copy link
Member

@styfle styfle commented Aug 9, 2023

This PR adds documentation for light/dark mode detection with next/image.

In the future, we could also document the picture solution once #51205 goes stable (although preloading would still be a problem).

@ijjk ijjk added examples Issue/PR related to examples created-by: Next.js team PRs by the Next.js team labels Aug 9, 2023
@styfle styfle marked this pull request as ready for review August 9, 2023 00:31
@styfle styfle requested review from timneutkens, ijjk, shuding, huozhi and a team as code owners August 9, 2023 00:31
@styfle
Copy link
Member Author

styfle commented Aug 9, 2023

After merging, we should double check that https://image-component.nextjs.gallery/theme deployed properly.

I set it up in #47130 to automatically deploy but its good to double check to avoid a broken link.

Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to document this or should it wait for the upcoming changes? It feels a bit strange to recommend the CSS display none change, felt like more of a hack than best practices, but maybe missing something.

docs/02-app/02-api-reference/01-components/image.mdx Outdated Show resolved Hide resolved
Co-authored-by: Lee Robinson <me@leerob.io>
@styfle
Copy link
Member Author

styfle commented Aug 9, 2023

Do we want to document this or should it wait for the upcoming changes?

If you are referring to #51205, then I think it actually ends up being a lot more code and a lot more complex then the example in this PR. That is really only needed for Art Direction.

We already know that CSS like this works because we've been using it in production for several years. So its best to document and explain how it works since this question comes up a lot.

Furthermore, most apps that use dark/light mode already have detection outside the image, especially if they allow the user to toggle between light/dark because that would be stored in a cookie. So a simple media query may not be enough, but dynamic css is sufficient for all cases.

@styfle styfle requested a review from leerob August 9, 2023 14:00
styfle and others added 2 commits August 9, 2023 10:04
@styfle styfle requested a review from leerob August 9, 2023 14:40
@kodiakhq kodiakhq bot merged commit 8730183 into canary Aug 9, 2023
52 checks passed
@kodiakhq kodiakhq bot deleted the styfle/next-1501-docs-for-dark-mode-theme-switching-for-nextimage branch August 9, 2023 14:55
kodiakhq bot pushed a commit that referenced this pull request Aug 10, 2023
Follow up to #53760

This follows the same pattern as other demo links on this page.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Next.js team PRs by the Next.js team examples Issue/PR related to examples locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants