graphql / graphiql

GraphiQL & the GraphQL LSP Reference Ecosystem for building browser & IDE tools.
MIT License
16.05k stars 1.72k forks source link

[graphiql/react] useTheme hook does not update when user changes the theme #2956

Open andreaforni opened 1 year ago

andreaforni commented 1 year ago

Is there an existing issue for this?

Current Behavior

Please have a look at this Codesandbox example

I created a TestPlugin that uses the useTheme hook to get the current theme. When the plugin loads, it gets the correct theme, but if the user changes it, the value returned by the hook does not update. I need to close and reopen the plugin tab to get the updated value.

Please, have a look at the following video:

https://user-images.githubusercontent.com/1233582/206126554-715784c9-2f07-469f-9f7d-f13a5f0dce0c.mov

Expected Behavior

The value returned by useTheme hook should update when the user changes the theme in the settings.

Steps To Reproduce

No response

Module pattern

Environment

- GraphiQL Version: 2.2.0
- OS:
- Browser: Chrome
- Bundler:
- `react` Version: 18.0.0
- `graphql` Version: 16.6.0

Anything else?

No response

andreaforni commented 1 year ago

As a side note, I've noticed that if the user selects "System" theme, the useTheme hook returns an empty string instead of light or dark. In this way, I cannot know which theme is set.

React_App
dimaMachina commented 1 year ago

As a side note, I've noticed that if the user selects "System" theme, the useTheme hook returns an empty string instead of light or dark. In this way, I cannot know which theme is set.

@thomasheyenbrock @jonathanawesome useTheme should returns theme: 'dark' | 'light' | 'system' and resolvedTheme: 'dark' | 'light' saw it in next-themes and it is very useful https://github.com/pacocoursey/next-themes#usetheme-1

matijagaspar commented 1 year ago

@B2o5T

As a side note, I've noticed that if the user selects "System" theme, the useTheme hook returns an empty string instead of light or dark. In this way, I cannot know which theme is set.

@thomasheyenbrock @jonathanawesome useTheme should returns theme: 'dark' | 'light' | 'system' and resolvedTheme: 'dark' | 'light' saw it in next-themes and it is very useful https://github.com/pacocoursey/next-themes#usetheme-1

Regarding the resolvedTheme:

Basically one needs to add a listener to the matchMedia.

In our plugin we made a custom hook using MUIs useMediaQuery https://github.com/nearform/mercurius-federation-info-graphiql-plugin/blob/master/src/federation-info-plugin/hooks/useGraphiqlTheme.js

But I am not 100% sure graphiql should handle this, because graphiql itself it doesn't need this functionality (yet?), it feels unnecessary to have an event listener for that just as an utility?

on the dark | 'light' | 'system' I aggree, but mind that it would potentially be a "breaking" change (if any plugin relied on it already)