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

use next-themes for handling theme #712

Merged
merged 8 commits into from
Feb 14, 2024

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Feb 9, 2024

Description

Background #707, This works nicely and also fixes the flickering issue.

next-themes seems like an popular library and used by ShadCn and NextUI

This also fixes #709

Test App : https://usehooks-ts-test.vercel.app/

the only downside is currently it does not hot react to theme changes of system preference (at the OS level). Although can be easily handled if we give the user an extra choice "System" instead of only 2 :
Screenshot 2024-02-09 at 7 40 51 PM

PS: It defaults to user's OS theme when user very first time visit the website, after that if he switches the theme manually then it doesn't react to OS changes

Copy link
Collaborator

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

Really nice job @technophile-04 exploring this option! 🙌

It's working great to me in both Dark and Light OS 👌

I had a super small issue in your deployed example, where the day/night radio button was moving a few pixels above the initial render when doing F5. In localhost it was working fine, and when I deployed to Vercel it was working fine too. Not big deal anyways, was only happening when forcing the refresh, not when changing tabs.

@technophile-04
Copy link
Collaborator Author

technophile-04 commented Feb 10, 2024

I had a super small issue in your deployed example, where the day/night radio button was moving a few pixels above the initial render when doing F5.

Ohh yeah, actually saw it was not happening on localhost more importantly on chains.hardhat because we have Faucet button and BlockExplorer buttons present always there, when I deployed to vercel I switched to sepolia because of which the ETH price button was popping up a bit late which was causing SwitchTheme to change its height

Screen.Recording.2024-02-11.at.1.57.09.AM.mov

But fixed at 677cd37 by giving it some definite height so that it doesn't change

Here is test app https://usehooks-ts-test.vercel.app/

Thanks Pablo 🙌

@Pabl0cks
Copy link
Collaborator

Ohh yeah, actually saw it was not happening on localhost more importantly on chains.hardhat because we have Faucet button and BlockExplorer buttons present always there, when I deployed to vercel I switched to sepolia because of which the ETH price button was popping up a bit late which was causing SwitchTheme to change its height

Nice catch and fix!

@rin-st
Copy link
Member

rin-st commented Feb 11, 2024

completely agree with you here #707 (comment)

At the beginning, I thought it's not worth using the whole library just to handle the theme but digging a bit deep in the NextJS context I think its worth using due to all the caveats present, and handling them ourselves properly might be a pain.

tested it and works great for me
great job as always!

@technophile-04 technophile-04 merged commit e3f76b8 into up-usehooks-ts Feb 14, 2024
@technophile-04 technophile-04 deleted the usehooks-ts-next-themes branch February 14, 2024 12:57
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.

3 participants