-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
Add option to enter endpoint URL (fix #116) #148
base: main
Are you sure you want to change the base?
Add option to enter endpoint URL (fix #116) #148
Conversation
demo/IntrospectionModal.tsx
Outdated
onChange={this.handleURLChange} | ||
/> | ||
<div> | ||
Paste headers (in JSON format) into the textarea below. |
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.
Great for PoC but it bad UX in the long run, it would be constant source of frustration for users.
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 believe this is an acceptable solution. Please take a look at GraphQL Playground. Users paste headers in GraphQL Playground in JSON format. GraphQL Playground is production-ready tool.
Also users paste variables in GraphiQL and GraphQL Playground in JSON format too. The target audience of these tools are developers. So I think this is ok.
demo/IntrospectionModal.tsx
Outdated
switch (inputType) { | ||
case Presets: | ||
this.props.onChange(PRESETS[activePreset]); | ||
break; | ||
case URL: | ||
this.fetchIntrospection(urlText, headers).then((data) => this.props.onChange(data)); |
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.
How UI look like during fetch?
and where is error handling?
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.
UI looks the same as for the rest "Introspection" and "SDL" tabs. Please note, there is no any error handling for them too. E.g.:
- If the user has entered invalid JSON or SDL into textarea, there is no any UI toast about JSON parsing or introspection from schema building errors.
- If the user has not entered JSON or SDL into textarea at all, the "Display" button is still active.
Errors are printed in the browser console.
@IvanGoncharov Thanks for your comments! They are all great points. But I don't plan to fix them as part of this pull request because they are out of scope. I'm guided by Google's Engineering Practices. TL;DR: The product/code should get better than before merging a change, even if it isn’t perfect. Before merging my change, users couldn't enter GraphQL endpoint URLs at all. After merging, they could. So the product is in better state than before. The quality of my change is not lower than the quality of the code. So it doesn't degrade the overall code health. Nevertheless, I would be happy to fix them (and some other UI/UX issues I found) in the next iteration as a separate pull request. 😉 |
Deployed a live demo: https://graphql-voyager.web.app. |
Updated live demo: https://graphql-voyager.web.app. |
This is an excellent feature that helps navigate the graph especially when there are multiple services which are federated. We have a use case where we would like to specify URL and let the voyager render the graph. Appreciate if this can be merged. |
Hey @IvanGoncharov, what's the state of this feature request? If the MR is too old to be merged I'll be happy to open a new mr with those changes (and we can discuss which changes to implement). |
This pull request adds new "URL" tab into "Change schema" dialog to enter endpoint URL.
Before:
After: