streamich / react-use

React Hooks — 👍
http://streamich.github.io/react-use
The Unlicense
41.57k stars 3.14k forks source link

useDark #1931

Open wwwenjie opened 3 years ago

wwwenjie commented 3 years ago

I think we can add useDark hook to manage the prefers color scheme.
Here's an example of vueuse.

Behavier: it reads the value from localStorage/sessionStorage(the key is configurable) to see if there is user configured color scheme, if not, it will use users' system preferences. When you change the isDark ref, it will update the corresponding element's attribute and then store the preference to storage for persistence.

If it could be added, I'm willing to contribute to react-use.

deadcoder0904 commented 3 years ago

Check https://github.com/xcv58/use-system-theme which does the same thing if you're looking for inspiration.

wwwenjie commented 3 years ago

I think the libray you mentioned is good for get system theme. however, useDark hook should expose more feature like toggle color sheme, manage color sheme in localstorage. I write a simple useDark hook in here: https://github.com/wwwenjie/react-starter/blob/main/src/hooks/useDark.ts

Should react-use add this hook? Before make a PR I want to know if useDark is necessary for react-use.

Svish commented 3 years ago

useDark sounds to me too specific (would be better with useTheme or something like that), but also like a "bigger thing" that would be better dealt with in a focused package, rather than mixed in with small utility hooks.

deadcoder0904 commented 3 years ago

@Svish agreed but let's be honest, nobody is making more than 2 themes so light & dark seems good to me (see https://twitter.com/adamwathan/status/1291717970658570240 & https://twitter.com/adamwathan/status/1300493628075388929)

@wwwenjie at this point, you should just use next-themes instead. It reduces your code by so much. I had used use-system-theme with mobx to configure light & dark theme but next-themes reduced the boilerplate by so much (see https://github.com/leerob/leerob.io/blob/62215bb79483b6a26e2e7e06c4141f540b67efe5/pages/_app.js#L12 & https://github.com/leerob/leerob.io/blob/b1e44e1e84d06e30bf438de6b495166ca748a5ef/components/Container.js#L11)

Svish commented 3 years ago

@deadcoder0904 Sure, I just think the use of dark in the name forces light being the default on you. not sure I like that about tailwind either. but yeah, light and dark themes is the most common, and for a system like tailwind it totally makes sense to limit it to those two.

For a generic react hook on the other hand, I'm not so sure it makes sense to "lock it" to just dark and light.

deadcoder0904 commented 3 years ago

@Svish no it's not about Tailwind at all. He is saying that there are not as many applications that use themes other than dark & light. It's too much work even to support 2 themes. Like 99% apps use just light mode. Out of that 1%, 99% have light & dark mode.

Svish commented 3 years ago

I don't see how that changes anything. My point is just that for a generic theme hook, in a generic react hooks library, it makes no sense to me to only support "dark" and "light". Even if it's only 2 themes, the names shouldn't matter to the hook. What if I wanted them to be "white" and "black" instead? Or "pastel" and "heavy"?

Either way I'm not sure a useDark or useTheme makes sense in a library like this. There are several other hooks I also don't think should be in this package though, so maybe just me...

Svish commented 3 years ago

If anything, it should be useColorScheme or something like that, to reflect the actual media query it's based on. With dark and light as values.

deadcoder0904 commented 3 years ago

I don't see how that changes anything. My point is just that for a generic theme hook, in a generic react hooks library, it makes no sense to me to only support "dark" and "light". Even if it's only 2 themes, the names shouldn't matter to the hook. What if I wanted them to be "white" and "black" instead? Or "pastel" and "heavy"?

Agreed. I was just ranting about creating more than 2 themes is not worth it. But you're right.

There are several other hooks I also don't think should be in this package though, so maybe just me...

Not just you but what can we say.