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

GraphiQL component: forcedTheme #3407

Merged
merged 6 commits into from
Jun 17, 2024
Merged

Conversation

TuvalSimha
Copy link
Contributor

@TuvalSimha TuvalSimha commented Aug 22, 2023

closes issue: #3398

Description:

  • User can force theme - added forcedTheme prop
  • When forcedTheme is set, the toggle theme option in the setting disappears.

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2023

🦋 Changeset detected

Latest commit: b7e6f18

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
graphiql Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@dimaMachina dimaMachina left a comment

Choose a reason for hiding this comment

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

Can enforce the theme from editorTheme prop.

so if the user can enforce the theme via editorTheme why do we need additional prop showThemeSettings?

We could show the theme toggle only if props.editorTheme was not set

Updated: let's stay with new prop forcedTheme

Also please add simple cypress tests

@TuvalSimha TuvalSimha changed the title GraphiQL component: showThemeSettings GraphiQL component: forcedTheme Aug 23, 2023
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Attention: Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 55.78%. Comparing base (c126878) to head (e0da0ab).

Current head e0da0ab differs from pull request most recent head b7e6f18

Please upload reports for the commit b7e6f18 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3407      +/-   ##
==========================================
- Coverage   60.80%   55.78%   -5.03%     
==========================================
  Files         120      110      -10     
  Lines        5616     5265     -351     
  Branches     1487     1434      -53     
==========================================
- Hits         3415     2937     -478     
- Misses       1749     1904     +155     
+ Partials      452      424      -28     
Files Coverage Δ
packages/graphiql/src/components/GraphiQL.tsx 66.44% <8.33%> (-2.27%) ⬇️

... and 31 files with indirect coverage changes

@mvdiener
Copy link

mvdiener commented Aug 23, 2023

Hello, I'm not an active contributor to this repo, but I am actively watching these changes because I'm very interested in utilizing this property.

I'm wondering if these changes still allow for the utilization of the system theme? In the code for the useTheme hook, the theme value of null indicates the theme should use the system default. Does the new forcedTheme property allow for the scenario where we want to hide the theme toggle in the settings modal, but also want the theme to be set based on the system default?

Perhaps a third option for theme: forcedTheme?: 'light' | 'dark' | 'system'; and under the hood, 'system' can be translated as null when calling setTheme()?

Thank you, and looking forward to these upcoming changes!

@dimaMachina
Copy link
Collaborator

dimaMachina commented Aug 24, 2023

@mvdiener i guess we can use undefined value to disable forcedTheme and null to set as theme system and migrate to next-themes package in v4 which has system that is aka null now (in our useTheme hook)

Update : or even already use system that will be alias to current null

changeset

change prop to enforceTheme

clean

change prop name

cypress theme tests

delete onEditForceTheme

fix some

new changeset

fix

fix

Update packages/graphiql/cypress/e2e/theme.cy.ts

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/cypress/e2e/theme.cy.ts

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/cypress/e2e/theme.cy.ts

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/resources/renderExample.js

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/src/components/GraphiQL.tsx

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/src/components/GraphiQL.tsx

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/cypress/e2e/theme.cy.ts

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update .changeset/famous-shirts-mate.md

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/cypress/e2e/theme.cy.ts

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/cypress/e2e/theme.cy.ts

Co-authored-by: Dimitri POSTOLOV <[email protected]>

Update packages/graphiql/cypress/e2e/theme.cy.ts

Co-authored-by: Dimitri POSTOLOV <[email protected]>
@louisscruz
Copy link

Are there any additional adjustments needed to get this in? cc @B2o5T

@dimaMachina dimaMachina merged commit 115c1c0 into graphql:main Jun 17, 2024
11 of 12 checks passed
@acao acao mentioned this pull request Jun 17, 2024
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.

5 participants