-
Notifications
You must be signed in to change notification settings - Fork 387
Add params tables for GraphQL and Rest Client docs. #123
Conversation
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.
👍🏻 Looking good! Couple little notes and considerations
docs/usage/graphql.md
Outdated
@@ -2,11 +2,21 @@ | |||
|
|||
Once OAuth is complete, we can use the library's `GraphqlClient` to make requests to the GraphQL Admin API in a similar way. To do that, create an instance of `GraphqlClient` using the current shop URL and session `accessToken` in your app's endpoint. | |||
|
|||
The `GraphQLClient`'s main method is `query`, which accepts a `GraphQLParams` object as its argument. `GraphQLParams` only requires the `data` parameter, but also optionally accepts `query`, `extraHeaders`, and `tries`: | |||
|
|||
| Parameter | Type | Notes | Required? | Default Value | |
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 think a good rule is to always put the "Notes" column last, since it's often the largest chunk and in some contexts might push more critical information (like "Required?") out of the user's view
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 agree. Also, I'm always torn between having a required?
column or having the parameters as data
/ query?
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.
@gfscott Oh that's a really good point, thank you! I'll shift that over.
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.
@paulomarg Yeah I feel the same way. Is it obvious enough to just end the param name in a ?
to mark it as optional? So few of the params are required it feels like a waste of table space. I actually feel the same way about the Default Value
column, but want to communicate the default for tries
in an obvious way.
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 great!!!
docs/usage/graphql.md
Outdated
@@ -2,11 +2,21 @@ | |||
|
|||
Once OAuth is complete, we can use the library's `GraphqlClient` to make requests to the GraphQL Admin API in a similar way. To do that, create an instance of `GraphqlClient` using the current shop URL and session `accessToken` in your app's endpoint. | |||
|
|||
The `GraphQLClient`'s main method is `query`, which accepts a `GraphQLParams` object as its argument. `GraphQLParams` only requires the `data` parameter, but also optionally accepts `query`, `extraHeaders`, and `tries`: | |||
|
|||
| Parameter | Type | Notes | Required? | Default Value | |
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 agree. Also, I'm always torn between having a required?
column or having the parameters as data
/ query?
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.
LGTM
c771fe5
to
96c3079
Compare
WHY are these changes introduced?
Partially fixes 121Strikethru the above to keep docs issue open, but this does partially resolve some of the needed docs enhancement outlined in that issue.
WHAT is this pull request doing?
Adds the necessary tables to explain the available params and arguments for the
GraphQL
andRest
clients, as well as general improvements to the usage examples.Type of change
- [ ] Minor: New feature (non-breaking change which adds functionality)- [ ] Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)Checklist
- [ ] I have added/updated tests for this change- [ ] I have documented new APIs/updated the documentation for modified APIs (for public APIs)