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

Adding Darkmode to graphiql #2146

Closed
wants to merge 5 commits into from
Closed

Adding Darkmode to graphiql #2146

wants to merge 5 commits into from

Conversation

harshitkumar31
Copy link
Contributor

Addresses #129
Screenshot 2022-01-21 at 8 58 12 PM

@changeset-bot
Copy link

changeset-bot bot commented Jan 21, 2022

🦋 Changeset detected

Latest commit: 53aeed1

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

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

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

@acao
Copy link
Member

acao commented Feb 2, 2022

@harshitkumar31 thanks!

Hoping to merge this soon - can you create a changeset as per the bot comment?

@acao
Copy link
Member

acao commented Feb 2, 2022

also, can you add it to GraphiQLProps and document the property? currently, it's only been added to GraphiQLState

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #2146 (53aeed1) into main (2d91916) will decrease coverage by 0.92%.
The diff coverage is 77.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2146      +/-   ##
==========================================
- Coverage   65.70%   64.78%   -0.93%     
==========================================
  Files          85       77       -8     
  Lines        5106     5188      +82     
  Branches     1631     1655      +24     
==========================================
+ Hits         3355     3361       +6     
- Misses       1747     1823      +76     
  Partials        4        4              
Impacted Files Coverage Δ
packages/codemirror-graphql/src/hint.ts 94.73% <ø> (ø)
packages/codemirror-graphql/src/lint.ts 100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts 47.05% <ø> (ø)
...kages/codemirror-graphql/src/utils/forEachState.ts 100.00% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts 0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/hint.ts 89.70% <ø> (ø)
packages/codemirror-graphql/src/variables/mode.ts 79.48% <ø> (ø)
packages/graphiql/src/components/QueryEditor.tsx 63.96% <ø> (ø)
packages/graphiql/src/utility/fillLeafs.ts 5.33% <ø> (ø)
...kages/graphiql/src/utility/introspectionQueries.ts 100.00% <ø> (ø)
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ccb3d8...53aeed1. Read the comment docs.

packages/graphiql/src/css/app.css Outdated Show resolved Hide resolved
packages/graphiql/src/components/GraphiQL.tsx Outdated Show resolved Hide resolved
packages/graphiql/src/components/GraphiQL.tsx Show resolved Hide resolved
@acao
Copy link
Member

acao commented Feb 2, 2022

looks great overall! again, thanks so much for this

@harshitkumar31 harshitkumar31 requested a review from acao February 2, 2022 20:23
@acao
Copy link
Member

acao commented Feb 2, 2022

that was fast!

@acao
Copy link
Member

acao commented Feb 2, 2022

Actually you know what - we might need to re-evaluate this strategy for 1.0 because it will invert GraphiQL.Logo which is still supported until 2.0, as well as images in markdown docs. I didn't realize filter would work on images! Can we use a different strategy?

@harshitkumar31
Copy link
Contributor Author

harshitkumar31 commented Feb 3, 2022

@acao , Added a style to ensure images aren't inverted.
For the lack of a better example, I added an image inline to test it. This should address your concern w.r.t GraphiQL.Logo
Screenshot 2022-02-04 at 12 53 17 AM

@acao
Copy link
Member

acao commented Feb 3, 2022

thank you! looks great

@acao
Copy link
Member

acao commented Feb 3, 2022

Hmmm... doc explorer font colors... I don't know... hmm!

@brucePedroGomes
Copy link

Hi, how you doing ? what should be changed? Can I help you?

@acao
Copy link
Member

acao commented Feb 5, 2022

I don’t think the css filter invert() approach will work. It would be awesome if it did!

This approach makes all the colors look off, and now the doc explorer might fail accessibility tests if dark mode is enabled because of the color contrast

There is a css file someone posted on gist in that thread that should be fine with some tweaking, but make sure it works for query history as well

in the new graphiql rfc as you can see we dont use CSS files, we use theme ui & emotion css, so you don’t have to worry about this working in 2.0 because we will use a different technique for accessing dark/light/solarized and other filtered theme modes, as well as user theme selection. User just provides a set of theme object literals. This is why we were waiting for 2.x to do this but no one is working on graphiql or has time

@harshitkumar31
Copy link
Contributor Author

I see. I don't have enough cycles or knowledge to implement the suggested changes. I'll close the PR, thanks.

@harshitkumar31 harshitkumar31 deleted the darkMode branch February 6, 2022 08:38
@acao
Copy link
Member

acao commented Feb 6, 2022

Well, you don’t need to close it in my opinion. I am glad to help you with this PR. I appreciate your initiative immensely, and a dark mode would be a great addition to wrap up 1.0

I completely forgot that a major part of this strategy should involve dynamically changing the codemirror theme as well!

This would make the more manual CSS override approach more practical I think?

@acao
Copy link
Member

acao commented Feb 6, 2022

It could be that we also get filter: invert() working where the doc explorer items look funky now (try doc search as well). Probably just a few more css selectors is all we need

@acao
Copy link
Member

acao commented Feb 6, 2022

How about I create a new PR with your commit and my changes? That way you get credit for the original approach. I think we can make this work even if you don’t have time to finish. I think this will work out just fine!

@harshitkumar31 harshitkumar31 restored the darkMode branch February 6, 2022 13:36
@harshitkumar31 harshitkumar31 reopened this Feb 6, 2022
@harshitkumar31
Copy link
Contributor Author

Sounds good. Thanks @acao

@thomasheyenbrock
Copy link
Collaborator

thomasheyenbrock commented May 9, 2022

As part of the designs for GraphiQL v2 we'll ship a dedicated dark mode implementation, for updates see #2328. I'm closing this PR in favor of the upcoming dark mode implementation. Thanks for everyone contributing here! ❤️

@harshitkumar31 harshitkumar31 deleted the darkMode branch July 19, 2022 12:49
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.

4 participants