Skip to content

feat: use client-hints for theme#479

Merged
kentcdodds merged 16 commits intokentcdodds:mainfrom
kishanhitk:client-hints-theme
Oct 30, 2023
Merged

feat: use client-hints for theme#479
kentcdodds merged 16 commits intokentcdodds:mainfrom
kishanhitk:client-hints-theme

Conversation

@kishanhitk
Copy link
Contributor

@kishanhitk kishanhitk commented Oct 10, 2023

Use Client Hints polyfill from The Epic Stack to get the user's theme preference.

Ref: https://github.com/epicweb-dev/epic-stack/blob/main/docs/client-hints.md
Ref: https://x.com/kentcdodds/status/1711082060046373174

@kishanhitk
Copy link
Contributor Author

Should we also add system theme mode, like we have in the epic stack example?
Or let it be only expliciy light and dark mode like we have currently?

@kentcdodds
Copy link
Owner

I would definitely like to support system

@kishanhitk
Copy link
Contributor Author

To be more specific:
Here are the two approaches:

  1. EpicStack example: The theme selector button has three options: dark, light, and system.
    If you choose either light or dark, then your system theme mode won’t be respected.
    But, if you choose the system mode, it will follow the OS’s theme mode.

  2. kentcdodds.com's current implementation: The theme selector allows only two options: light and dark.
    But it also respects your system theme mode by default. This means no matter what you selected explicitly from the UI, if you change your OS’s theme later, the website will follow it. To be more clear, the latest toggle will be followed, whether it is from the website's theme selector or a change of OS's theme.

Which approach do you prefer? I suggest the 2nd because it will fit perfectly with the current theme selector animation (the sunrise and moonrise animation).

@kentcdodds
Copy link
Owner

Yeah, but the 2nd functionality is also unclear. I definitely want the EpicStack's implementation. If you can come up with a nice improvement to the animation that's great. Or you can do a dropdown like the Epic Stack's profile button does.

@kishanhitk kishanhitk marked this pull request as ready for review October 13, 2023 17:28
@kishanhitk
Copy link
Contributor Author

This is what the new animation looks like:

Screen.Recording.2023-10-13.at.11.00.01.PM.mov

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is looking great! Just a few things.

Thanks a lot!

@kishanhitk
Copy link
Contributor Author

Thanks @kentcdodds,
I have resolved all the changes you asked for.

P.S.- Sorry it took so long; I was on vacation.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you! This is a huge help :)

@kentcdodds
Copy link
Owner

Looks like there are typescript issues. I don't think they're related to your changes. The unified ecosystem can be a bit of a mess sometimes. They just had a v3 release so I'm guessing these are related to that :(

@kentcdodds kentcdodds merged commit e488c74 into kentcdodds:main Oct 30, 2023
@kentcdodds
Copy link
Owner

Thank you so much for making this contribution. The fact my website didn't have a system mode really bugged me and I never liked my theming hacks either. I'm definitely happier with this now :) Thanks!

})();
`

function handleDarkAndLightModeEls() {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm so glad this is gone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants