-
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
feat: Add @graphiql/plugin-code-exporter
#2758
Conversation
🦋 Changeset detectedLatest commit: 56e56f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
@LekoArts this is looking great, thank you! also, hello and nice to see you again, welcome! i think all you need to do is to use |
Hey 👋 Nice to see you again, too 😊 I think I wired up all the props correctly, maybe @thomasheyenbrock sees an issue. But right now I'm trying to debug the code exporter itself as it somehow fails there: |
Figured it out, somehow the Do I need to set the defaults in here? |
Current state of things: Bildschirmaufnahme.2022-09-08.um.13.35.50.mov |
Which icon set do y'all use and which icon should this plugin get? |
Hey @LekoArts, this is awesome! You caught me heading out on vacation until end of next week, so I won't be able to give this a proper review until then.
I'm actually not sure if the icons GraphiQL uses for docs and history were custom-made by @julianbauer (who created the new design) or if they are coming from any library. For the explorer plugin I just picked a random one that I found online that kinda matched the others. |
Ok, good to know. Enjoy your vacation! Then I'll pick an icon (with proper license) to my best judgement :D |
Added the |
@thomasheyenbrock I'll be on vacation next week, are you back? From my side everything would be finished |
Hey @LekoArts 👋 I'm back since this week. I'll have time tomorrow to give this a review, but it already looks amazing! 😍 |
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@thomasheyenbrock Anything I can help with to get this reviewed? |
@thomasheyenbrock @acao ICYMI |
@LekoArts sorry that this has been stuck for a while and it's my fault, I dropped the ball on this one. I currently only have time one day every other week to really focus on GraphiQL, so Friday this will be my first thing to review 🤞 |
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.
Awesome work 👏 just some minor stuff, but apart from these this is looking good!
@@ -0,0 +1 @@ | |||
// / <reference types="vite/client" /> |
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.
We had an issue with out eslint config which always broke triple-slash directives (should by fixed by now)
// / <reference types="vite/client" /> | |
/// <reference types="vite/client" /> |
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.
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.
It's also like that in the other packages so I don't think this was fixed
export type GraphiQLCodeExporterProps = { | ||
query: string; | ||
snippets: Array<Snippet>; | ||
codeMirrorTheme: string; | ||
variables?: string; | ||
context?: Record<string, any>; | ||
schema?: GraphQLSchema | null | undefined; | ||
}; |
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.
These are different to the prop definitions that I find in the source repository:
codeMirrorTheme
is optional in the source repo but required herevariables
andcontext
are required in the source repo but optional here- A bunch of other props from the source repo are just missing here
Are there particular reasons for deviating from the prop definitions? Could we try to mirror them here instead of just declaring a subset?
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.
codeMirrorTheme is optional in the source repo but required here
With the ref handling for some reason the default props weren't applied. So unless (with the current code) you define the codeMirrorTheme
explicitly, the whole thing didn't work.
Maybe when I'll remove the wrapper it works, I'll test it.
variables and context are required in the source repo but optional here
But they are set with {}
defaults. So actually their types are incorrect and they are optional.
A bunch of other props from the source repo are just missing here
I didn't find them useful at the time, as I just wanted to make something minimal working. But I can copy the types, yeah.
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.
Also the props differ from README to code 😆
https://github.com/OneGraph/graphiql-code-exporter#props
Thanks for the comments! I'll adjust the PR shortly :) |
Some more UI feedback after checking out the branch locally (screenshots and GIFs attached):
|
- deduplication - vite bundle config - move package to devDep - remove wrapper react component - eslint thingy
Co-authored-by: Thomas Heyenbrock <[email protected]>
Where is that close button coming from and how I can enable it for testing? |
Bildschirmaufnahme.2022-10-14.um.12.59.10.movFor me the pane hasn't a fixed width (on Chrome and Firefox). I set it to |
Fixed the dropdown styles Bildschirmaufnahme.2022-10-14.um.13.18.39.mov |
FYI, for now I copied the code from here to gatsbyjs/gatsby#36541 and at the moment we just publish it ourselves. I'd still like to use this package once published instead of maintaining our own version :) |
I think that didn't happen for you because you also used the explorer plugin at the same time. Without the stylesheet from that plugin the styles are missing. I just did some finishing touches here and already added that part. |
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.
Thanks for this awesome plugin and your patience @LekoArts 🙇 gonna merge and publish this today!
...if CI doesn't act up. Seems like unpkg has some issues 🤷 Loading |
Works again! CI is happy ✅ |
This is released now as |
Thanks @thomasheyenbrock 👍 I'll then update our internal usage to this plugin instead :) |
Description
Adds https://github.com/OneGraph/graphiql-code-exporter, copying what #2715 did.
Only "problems" are:
@graphiql/react
componentsWith a bunch of CSS overrides it works fine now.
Preview
Bildschirmaufnahme.2022-09-08.um.13.35.50.mov
Related Issues
Doing that to unblock myself for gatsbyjs/gatsby#36541