-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Conversation
@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. |
The path |
@LekoArts yeah I was thinking the same, that is why I kept GraphiQL on |
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. |
Ahh got it, will do that 👍🏽 |
@LekoArts done |
@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. |
Thank you^^ |
@Saifadin Yea, there is space for improvements, but basically code editor follows basic |
@HuVik Glad to help, if you can point me to a list of main issues :) |
Co-Authored-By: Saifadin <osamah@aldoaiss.de>
I think accessibility might be an issue here cc @fk |
@sidharthachatterjee can you elaborate? Would love to hear your concerns in more detail. |
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 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. ThemingTheme 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 :) |
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:
ThemingIf 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 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 |
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 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. |
Thanks a lot @fk ^__^ When we created the theme in Sketch it showed us 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. |
That sentence didn't come out correctly, and I forgot to mention that I think that I do not see Super cool to hear that you went through the effort of using Sketch to design the theme! |
All good ^^ |
@@ -92,13 +94,19 @@ async function startServer(program) { | |||
heartbeat: 10 * 1000, | |||
}) | |||
) | |||
app.use( | |||
app.post( |
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.
This is a breaking change since GET
requests to /___graphql
with Accept: application/json
which currently work will break as a result of this
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.
I was not aware of that, good catch.
But then I am confused about how I can use the same Url for two get
s. Once for playground UI and for the GET
requests.
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 you have another look, it should work now
Added to the documentation What is the status here? |
@Saifadin There is a new version of the playground, could you please upgrade middleware version? 😇 |
Need some time to verify the change locally. Code looks good. |
Co-Authored-By: Saifadin <osamah@aldoaiss.de>
Co-Authored-By: Saifadin <osamah@aldoaiss.de>
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.
Verified locally, and works as expected. Thanks @Saifadin for this PR!
It was a pleasure 🎉 |
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:
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! |
Description
A PR that adds the Prisma Playground to the Gatsby Build process alongside GraphiQL.
Related Issues
#11120
TO-Dos
graphql-playground-middleware-express
/___playground
route