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.

Sign up for GitHub

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

Jump to bottom

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

Open
1 of 4 tasks
andreaforni opened this issue Dec 7, 2022 · 3 comments · May be fixed by #2971
Open
1 of 4 tasks

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

andreaforni opened this issue Dec 7, 2022 · 3 comments · May be fixed by #2971
Labels
bug graphiql

Comments

@andreaforni
Copy link

andreaforni commented Dec 7, 2022 •
edited
Loading

Is there an existing issue for this?

  • I have searched the existing issues

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:

Screen.Recording.2022-12-07.at.09.21.52.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

  • graphiql-umd
  • graphiql-esm
  • graphiql-commonjs

Environment

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

Anything else?

No response
This was referenced Dec 7, 2022
Merged
Closed
@andreaforni
Copy link
Author

andreaforni commented Dec 7, 2022 •
edited
Loading

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

@matijagaspar matijagaspar linked a pull request Dec 12, 2022 that will close this issue
Open
@dimaMachina
Copy link
Collaborator

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
Copy link

matijagaspar commented Dec 19, 2022 •
edited
Loading

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug graphiql
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(graphiql-react): add useTheme to context matijagaspar/graphiql
3 participants
@andreaforni @dimaMachina @matijagaspar