feat: Add url details and copy to cURL button#4220
Conversation
|
| return httpFetcher(graphQLParams, fetcherOpts); | ||
| }; | ||
|
|
||
| return Object.assign(fetcher, { [FETCHER_OPTIONS_SYMBOL]: options }); |
There was a problem hiding this comment.
We don't have a great way to access options provided to the fetcher since all the GraphiQL component sees is a function. I went with a symbol approach which lets us read those values in the component without the need for additional props.
That said, are there situations where this function isn't used? I hide the new UI when we can't detect these options on the fetcher function, but I'm wondering if we need to have some sort of prop on GraphiQL itself as replacement in case custom implementations provide fetchers that don't use this function.
I'm open to other ideas for sharing this information, but this was the best I could think of without introducing a breaking change.
There was a problem hiding this comment.
This seems reasonable to me. I would say maybe just leave as-is, and if there's a good place to document the limitation that'd be good.
There was a problem hiding this comment.
Great! That should be enough for me to write a changeset and add additional documentation. Thanks!
| @@ -0,0 +1,26 @@ | |||
| .url-details { | |||
| variableEditor?.getValue(), | ||
| ); | ||
|
|
||
| const headers: Record<string, string> = { |
There was a problem hiding this comment.
GraphQL Playground adds a whole lot more header values than I've included here, but I chose to keep this minimal for now. Let me know if we should include more.
| className="graphiql-tab-add" | ||
| onClick={addTab} | ||
| aria-label={LABEL.newTab} | ||
| <div className="graphiql-main-container"> |
There was a problem hiding this comment.
The "Hide whitespace" setting is recommended while reviewing this component.
|
Not a strong opinion here (and I think your placement is reasonable given playground's), but I'd imagined this as a button alongside the others in the editor (near prettify, etc.). Any preference between the two options? |
|
I thought about that too, though I struggled to figure out a good icon to use that would convey what it does, so I took the easy way out 😅. I didn't want it confused with the copy query button. Any ideas on what icon makes sense I moved it with the others? |
| DOC_EXPLORER_PLUGIN, | ||
| } from '@graphiql/plugin-doc-explorer'; | ||
| import { GraphiQLLogo, GraphiQLToolbar, GraphiQLFooter, Sidebar } from './ui'; | ||
| import { FETCHER_OPTIONS_SYMBOL } from '@graphiql/toolkit'; |
There was a problem hiding this comment.
Build is failing because toolkit is listed as a dev dependency which is actually pretty surprising to me. I'm curious if that was an intentional decision. Maybe @dimaMachina knows more.
|
@jerelmiller how about a terminal icon? |
|
That works for me! I'll try and push an update tonight to try moving that button over. Thanks for the suggestion! |
|
FYI this'll need a rebase against |
4f28e28 to
5097524
Compare
|
@trevor-scheer got those updates in place! I've added some new comments as followup questions as well that I'd love some input on. Thanks for the reviews thus far! |
| /** | ||
| * The GraphQL server url | ||
| */ | ||
| url?: string; |
There was a problem hiding this comment.
Should this be called fetcherUrl, or is url enough?
There was a problem hiding this comment.
No strong feelings, both seem reasonable. I haven't thought of another relevant URL that this needs to be disambiguated from, but I also haven't thought too hard about it :smiling:
| const copyAsCurl = ( | ||
| <ToolbarButton | ||
| label="Copy as cURL" | ||
| disabled={!url} |
There was a problem hiding this comment.
I opted to disable the button when the url is not available. Should we hide the component altogether instead?
There was a problem hiding this comment.
I like disabling. It'd be nice to explain why in the tooltip but might be a bit verbose, so I'm on the fence.
I'd say leave as is (and if you come up with some very concise copy for the disabled state, open to that).
trevor-scheer
left a comment
There was a problem hiding this comment.
Looks pretty good, a couple important comments to address within. Still needs a changeset as well. Thanks!
| operationName, | ||
| }); | ||
|
|
||
| const command = `curl '${url}' ${headersString} --data-binary '${data}'`; |
There was a problem hiding this comment.
I think we need to escape single quotes here. I'd extract this bit of the function:
toCurl(url, headers, data): string so we can easily unit test it.
| let operationName: string | undefined; | ||
|
|
||
| if (query) { | ||
| const definition = parse(query).definitions.find( |
There was a problem hiding this comment.
This can throw, and I think we have a couple options...
- on query update enable/disable the copy as curl button
- show something in the UI
- just log to console
- do nothing
Thoughts? 1 seems nice but maybe a bit heavy.
| const copyAsCurl = ( | ||
| <ToolbarButton | ||
| label="Copy as cURL" | ||
| disabled={!url} |
There was a problem hiding this comment.
I like disabling. It'd be nice to explain why in the tooltip but might be a bit verbose, so I'm on the fence.
I'd say leave as is (and if you come up with some very concise copy for the disabled state, open to that).
| /** | ||
| * The GraphQL server url | ||
| */ | ||
| url?: string; |
There was a problem hiding this comment.
No strong feelings, both seem reasonable. I haven't thought of another relevant URL that this needs to be disambiguated from, but I also haven't thought too hard about it :smiling:
Part of #3951
Adds a new URL bar that shows the GraphQL server URL along with a "Copy cURL" button.
Since this is my first PR, design suggestions welcome! I've added review comments to point out various details about this PR.
Screenshot