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

feat(www): Implement dark mode user system preference: `prefers-color-scheme: dark` #18348

Merged
merged 12 commits into from Oct 11, 2019

Conversation

@thomaswang
Copy link
Member

thomaswang commented Oct 8, 2019

Description

Add prefers-color-scheme: dark to /www/src/components/with-color-mode.js to support user system preference.

Good for the new OS's that have a user setting for dark mode. Instagram doesn't even have a toggle right now so we're a step ahead!

Related PR

Related to @fk awesome PR #18027

@thomaswang thomaswang requested a review from fk Oct 8, 2019
@thomaswang thomaswang requested review from gatsbyjs/learning as code owners Oct 8, 2019
@thomaswang

This comment has been minimized.

Copy link
Member Author

thomaswang commented Oct 8, 2019

thomaswang added 6 commits Oct 8, 2019
@fk

This comment has been minimized.

Copy link
Contributor

fk commented Oct 9, 2019

😍 Oh sweeeeeet, @thomaswangio! 👍 🏃 🙏

Sorry for not responding on Twitter yesterday (and still ;-)), got pretty late over here fixing some other dark mode related bugs. 😅
Unfortunately I also don't have the time to review this right now. :-(
Maybe someone can jump in?

@thomaswang thomaswang requested review from gillkyle and removed request for fk Oct 9, 2019
@thomaswang

This comment has been minimized.

Copy link
Member Author

thomaswang commented Oct 9, 2019

Maybe someone can jump in?

@gillkyle can you halp? 🥰

@lannonbr lannonbr changed the title [feat] Implement dark mode user system preference: `prefers-color-scheme: dark` feat(www): Implement dark mode user system preference: `prefers-color-scheme: dark` Oct 9, 2019
@gillkyle

This comment has been minimized.

Copy link
Contributor

gillkyle commented Oct 10, 2019

I've got this on my list to review (maybe tomorrow?)! Sorry for the wait!

@fk

This comment has been minimized.

Copy link
Contributor

fk commented Oct 10, 2019

Hey Thomas, hey Kyle!

I just realized theme-ui (iiuc) has us covered already <3:
https://theme-ui.com/color-modes#initialize-dark-mode-with-prefers-color-scheme-dark

@thomaswangio I only checked this briefly, wasn't even aware of this — until @johno's Tweet kinda rubbed my nose in it ;-)

Would you be willing to adjust your PR, and add useColorSchemeMediaQuery: true to the theme object? I'm really sorry for not realizing this up to now. I promise to approve ASAP! 🙄 😅 🙏

@thomaswang

This comment has been minimized.

Copy link
Member Author

thomaswang commented Oct 10, 2019

Hey @fk and @gillkyle, I tried adding the flag useColorSchemeMediaQuery: true to the /www/src/gatsby-plugin-theme-ui/index.js as such:

const config = {
  // this enables the color modes feature
  // and is used as the name for the top-level colors object
  initialColorMode: `light`,
  // `prefers-color-scheme: dark` media query
  useColorSchemeMediaQuery: true,
  // borders: borders,
  breakpoints: breakpointsTokens,
  colors: col,
  fonts: fontsTokens,
  fontSizes: fontSizesTokens,
  fontWeights: fweights,
  letterSpacings: ls,
  lineHeights: lineHeightsTokens,
  mediaQueries: mq,
  radii: r,
  shadows: sh,
  sizes: si,
  space: spaceTokens,
  transition: t,
  zIndices: z,
  buttons: {
    large: {
      fontSize: 4,
      px: 4,
      height: `52px`,
    },
    small: {
      fontSize: 2,
      py: 2,
      px: 3,
    },
  },
}

This doesn't work, however. Even removing initialColorMode: `light` it still doesn't work. The original solution of setting the preference in www/src/components/with-color-mode.js definitely works though!

@gillkyle

This comment has been minimized.

Copy link
Contributor

gillkyle commented Oct 11, 2019

That's a good catch @fk! And I suppose if that's having issues with the documented approach, it's probably something we ought to let @jxnblk know about so it could be caught upstream.

I'll pull your branch now and toy with it.

@thomaswang

This comment has been minimized.

Copy link
Member Author

thomaswang commented Oct 11, 2019

Ah thanks @gillkyle! I also noticed we're using the flag initialColorMode but the documentation says initialColorModeName. I'm not too familiar with Theme-UI, not sure if it's just a naming issue?

@gillkyle

This comment has been minimized.

Copy link
Contributor

gillkyle commented Oct 11, 2019

Okay! Looks like it works for me (the documented initialColorMode method that is), I've been setting the code just like you included in your snippet, then removing the value from local storage for theme-ui-color-mode. When I refresh the page after updating my OS setting it seems to work 🙂

In regards to the naming, it looks like initialColorMode was an older version of the name, and it's still supported as per this line: https://github.com/system-ui/theme-ui/blob/8ed1c5a8fd1d1c23809c3fb74b257a1517cf6611/packages/theme-ui/src/color-modes.js#L22 probably for backwards compatibility I'd imagine. Though we should probably use the documented version so people aren't confused when they look at how Gatsby does this.

I say update the name of initialColorMode and use the useColorSchemeMediaQuery key and we should be good!

@thomaswang

This comment has been minimized.

Copy link
Member Author

thomaswang commented Oct 11, 2019

@gillkyle done! thanks so much for testing that, I think the issue on my end was not clearing local storage. Should be good to go now 👍

Copy link
Contributor

gillkyle left a comment

Looks good to me, thanks @thomaswangio 🙂

@gillkyle

This comment has been minimized.

Copy link
Contributor

gillkyle commented Oct 11, 2019

Should probably wait for @fk to give a stamp of approval as well too, since he has done so much work on this 🙂

@fk

This comment has been minimized.

Copy link
Contributor

fk commented Oct 11, 2019

@fk
fk approved these changes Oct 11, 2019
@fk fk merged commit dab05c8 into gatsbyjs:master Oct 11, 2019
20 checks passed
20 checks passed
Danger All good
Details
Peril All green. Congrats.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 67 tests passed in 00:28
Details
unit_tests_windows Build #20191011.19 succeeded
Details
@thomaswang thomaswang deleted the thomaswang:prefers-dark-mode branch Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.