-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
🦋 Changeset detectedLatest commit: b7e6f18 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this 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
showThemeSettings
forcedTheme
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
|
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 Perhaps a third option for theme: Thank you, and looking forward to these upcoming changes! |
@mvdiener i guess we can use Update : or even already use |
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 <en3m@ya.ru> Update packages/graphiql/cypress/e2e/theme.cy.ts Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/cypress/e2e/theme.cy.ts Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/resources/renderExample.js Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/src/components/GraphiQL.tsx Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/src/components/GraphiQL.tsx Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/cypress/e2e/theme.cy.ts Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update .changeset/famous-shirts-mate.md Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/cypress/e2e/theme.cy.ts Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/cypress/e2e/theme.cy.ts Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru> Update packages/graphiql/cypress/e2e/theme.cy.ts Co-authored-by: Dimitri POSTOLOV <en3m@ya.ru>
a7f31d5
to
8f75b62
Compare
Are there any additional adjustments needed to get this in? cc @B2o5T |
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
closes issue: #3398
Description:
forcedTheme
propforcedTheme
is set, the toggle theme option in the setting disappears.