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

feat(gatsby): GraphQL playground #11193

Merged
merged 12 commits into from
Jan 29, 2019
Merged

Conversation

Saifadin
Copy link
Contributor

@Saifadin Saifadin commented Jan 21, 2019

Description

A PR that adds the Prisma Playground to the Gatsby Build process alongside GraphiQL.

Example Image

Related Issues

#11120

TO-Dos

  • Install graphql-playground-middleware-express
  • Add /___playground route
  • Add Gatsby Theme
  • Add to documentation

@Saifadin Saifadin requested a review from a team as a code owner January 21, 2019 15:59
@Saifadin
Copy link
Contributor Author

@sidharthachatterjee @HuVik here it is, will still add the Theme and the documentation. My colleague is making a contrast-rich theme design, that I will add, so it can be tweaked.

@LekoArts
Copy link
Contributor

LekoArts commented Jan 21, 2019

The path ___graphql should stay the same. You can replace GraphiQL with Playground but the former should stay the same - otherwise this would be a breaking change and a lot of articles/tutorials would be outdated.

@Saifadin
Copy link
Contributor Author

@LekoArts yeah I was thinking the same, that is why I kept GraphiQL on /___graphql and added /___playground or where you referring to the console.log? That one should mention both of them I think.

@LekoArts
Copy link
Contributor

No, what I meant is: Do a complete switch over to Playground but keep the same URL. That wouldn't be a breaking change IMO as just the interface changes. You can remove GraphiQL. At least that is what @freiksenet said.

@Saifadin
Copy link
Contributor Author

Ahh got it, will do that 👍🏽

@Saifadin
Copy link
Contributor Author

@LekoArts done

@Saifadin
Copy link
Contributor Author

Saifadin commented Jan 21, 2019

Added a Custom Theme for Gatsby, what do you guys think of this? We tried to stick to the Gatsby colors.

screenshot 2019-01-21 at 18 16 17

Another option would be this a little bit darker color scheme

screenshot 2019-01-21 at 18 19 33

@huv1k
Copy link
Contributor

huv1k commented Jan 21, 2019

@Saifadin It looks nice :) You can even change editor color scheme. And I see there is an old playground inside. There is right now hardcoded bad version of the playground, but I will remove it in the new release so there is always up to date version.

@Saifadin
Copy link
Contributor Author

Thank you^^
It was quite hard to find out which color goes where. Some kind of documentation would be beneficial, but I also noticed, that in the playground-html lib the type of codeTheme is incomplete. Made it harder to code 🤷🏽‍♂️

@huv1k
Copy link
Contributor

huv1k commented Jan 21, 2019

@Saifadin Yea, there is space for improvements, but basically code editor follows basic Codemirror theme, so any theme working with code mirror should works. If its remapped to object. But I agree right now there is a mess I should fix it asap.

@Saifadin
Copy link
Contributor Author

@HuVik Glad to help, if you can point me to a list of main issues :)

packages/gatsby/src/commands/develop.js Outdated Show resolved Hide resolved
packages/gatsby/src/commands/develop.js Outdated Show resolved Hide resolved
Co-Authored-By: Saifadin <osamah@aldoaiss.de>
@sidharthachatterjee
Copy link
Contributor

Added a Custom Theme for Gatsby, what do you guys think of this? We tried to stick to the Gatsby colors.

screenshot 2019-01-21 at 18 16 17

Another option would be this a little bit darker color scheme

screenshot 2019-01-21 at 18 19 33

I think accessibility might be an issue here

cc @fk

@Saifadin
Copy link
Contributor Author

@sidharthachatterjee can you elaborate?
The Colors in this Theme are all AAA from a color contrast.

Would love to hear your concerns in more detail.

@m-allanson
Copy link
Contributor

I haven't had a chance to try this out yet, but it looks great!

I'm not 100% sold on straight up replacing GraphiQL with Playground, at least not in a minor release. I also think including GraphiQL and Playground would be confusing. Could we do a gradual rollout, first by enabling Playground via an env var?

This would let you do GATSBY_GRAPHQL_IDE=playground gatsby develop to use Playground right now (well, not right now but as soon as this PR is released).

Then we could switch Playground to be the default in the next major release of Gatsby, before finally removing GraphiQL in a later version. I think this would give us a nice balance of enabling the new goodness for people that want it while letting us work through any issues and avoiding disrupting people's existing workflows.

Here's an example where we've done that for the node db that Gatsby uses internally.

Theming

Theme choices are always going to be somewhat subjective, for this PR I'd prefer to stick with the default theme. We can then use a follow up PR to work out a better theme, as the theme is not directly related to making this PR mergeable. This is also a nice benefit of enabling Playground behind a feature flag to start with - while it's feature flagged we can let people try it out and provide feedback, and don't need to worry too much about changing things until it's un-feature-flagged.

Personally I'd like a theme that's very similar to the existing GraphiQL theme. There are a lot of guides and tutorials about Gatsby that have GraphiQL screenshots in them, anything we can do to avoid them looking outdated is going to be an overall win for the community. But I'd be happy to be convinced that a light theme is a bad idea :)

@Saifadin
Copy link
Contributor Author

Concerning the gradual release, I kind of agree. It is too big a change for a minor release. A sudden change might be surprising, especially for people not closely following the news etc, so adding it behind an environment variables is a great way.

Basically these steps:

  1. Opt-In through environment variable and mentioning this possibility
  2. Deprecating GraphiQL in the next major release, while giving an opt-in possibility to it and updating docs with new images
  3. Next major release after that, remove it

Theming

If we use a gradual release, then I would put the custom theme aside for now and use the default.

Concerning light vs dark theme I think it subjective, but we can add a note in the docs, that it is changeable in the editor settings in the playground, by changing "editor.theme": "dark" to "editor.theme": "light".
If it is behind a feature flag, adding a dark theme is I think less of a problem. The moment we make it the default option, it might be necessary to make this discussion in detail.

So for now I am removing the custom theme from this PR, but I will be waiting for other feedback from the other Reviewers to the feature-flag discussion 😄

@LekoArts
Copy link
Contributor

So for now I am removing the custom theme from this PR, but I will be waiting for other feedback from the other Reviewers to the feature-flag discussion

Yeah, sorry for the confusion. What Mike said is probably the best solution to this

@fk
Copy link
Contributor

fk commented Jan 23, 2019

Great to see this happening @Saifadin!

Re: the theme — I love that you put in the work for a dark Gatsby theme (and took care about accessibility; I briefly checked the (blurry) JPGs and only saw AA contrast values for the two or three things I measured)!

That said, @m-allanson's comments regarding theming and the general plan to move forward make a lot of sense to me — there's enough work to take care of in terms of UX which does not concern the visual appearance. It makes sense to me to move incrementally — get the foundation done first, and then add the "theme" icing-on-the-cake in a separate step. This unblocks this PR from discussions regarding the latter.

I'd personally love to have both light and dark themes available.
Your working regarding the dark theme is a great foundation to build upon and I like to think it wasn't made in vain!

@Saifadin
Copy link
Contributor Author

Thanks a lot @fk ^__^

When we created the theme in Sketch it showed us AAA, but we would need to tweak it when we work on the theme in the future, for which I will create a second PR after this one is accepted.

Like I said, light and dark themes exist out of the box, which could also be indicated by an ENV variable maybe?

@m-allanson and the rest: If there are no big blockers, I will start implementing an env based solution today.

@fk
Copy link
Contributor

fk commented Jan 23, 2019

@Saifadin 🤗

I briefly checked the (blurry) JPGs and only saw AA contrast values for the two or three things I measured)

That sentence didn't come out correctly, and I forgot to mention that I think that I do not see AAA is due to the blurryness of the image. Sorry for making it sound like I wanted to say "the AAA contrast values you talked about aren't there" — that really wasn't my intention at all! 🙏

Super cool to hear that you went through the effort of using Sketch to design the theme!

@Saifadin
Copy link
Contributor Author

All good ^^
Not me, but my UI designer colleague did it together with me. He is ready to rework it in the later PR.^^

@LekoArts LekoArts changed the title Topics/playground feat(gatsby): GraphQL playground Jan 24, 2019
@@ -92,13 +94,19 @@ async function startServer(program) {
heartbeat: 10 * 1000,
})
)
app.use(
app.post(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change since GET requests to /___graphql with Accept: application/json which currently work will break as a result of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware of that, good catch.
But then I am confused about how I can use the same Url for two gets. Once for playground UI and for the GET requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you have another look, it should work now

@sidharthachatterjee
Copy link
Contributor

@Saifadin Looks like @fk addressed the accessibility concerns and shouldn't be an issue since we're releasing under a flag 🙂

Left a comment about the change from app.use to app.post being a breaking change

@Saifadin Saifadin requested a review from a team January 27, 2019 00:18
@Saifadin
Copy link
Contributor Author

Added to the documentation

What is the status here?

@huv1k
Copy link
Contributor

huv1k commented Jan 28, 2019

@Saifadin There is a new version of the playground, could you please upgrade middleware version? 😇

@pieh
Copy link
Contributor

pieh commented Jan 28, 2019

What is the status here?

Need some time to verify the change locally. Code looks good.

@Saifadin
Copy link
Contributor Author

@HuVik done :)
@pieh Got it 👍

@pieh pieh self-assigned this Jan 29, 2019
Co-Authored-By: Saifadin <osamah@aldoaiss.de>
Co-Authored-By: Saifadin <osamah@aldoaiss.de>
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Verified locally, and works as expected. Thanks @Saifadin for this PR!

@Saifadin
Copy link
Contributor Author

It was a pleasure 🎉

@pieh pieh merged commit 2b1b551 into gatsbyjs:master Jan 29, 2019
@gatsbot
Copy link

gatsbot bot commented Jan 29, 2019

Holy buckets, @Saifadin — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

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.

None yet

8 participants