Skip to content
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

DOC Update react injector docs to work #215

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Apr 12, 2023

Reapplies the changes to graphql injector docs that were (meant to be) reverted (along with any new changes I found that were required)

NOTE

The revert commit was not completed correctly. It was meant to revert the entirety of #187 and #197, but failed to do so!
That's okay because this commit would have partially reinstated those PRs anyway.

Make sure you test and review the entirety of the graphql/javascript code from ## Using Injector to customise GraphQL queries to the end of the file. That includes both the graphql 3 and graphql 4 sections! ALL of that is in scope for this PR.

Requires extra admin PR also

Issue

@GuySartorelli GuySartorelli force-pushed the pulls/4.13/injectable-graphql-again branch from 30d6f40 to 1a9525d Compare April 12, 2023 03:44
@GuySartorelli GuySartorelli changed the title DOC Update graphql injector docs DOC Update react injector docs to work Apr 12, 2023
@sabina-talipova
Copy link
Contributor

sabina-talipova commented Apr 14, 2023

Could we also fix this small spelling issue cient to client.

And small question: why we use somewhere app/client and in another places my-module/client?

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Apr 14, 2023

why we use somewhere app/client and in another places my-module/client?

It's to distinguish between the two contexts this documentation is bouncing between - there's a module which provides the notes components and the actual DataObject implementation, and then there's some app project which is customising that component by adding the priority field.

Really this whole section of docs needs to be reconsidered, probably being split into a more traditional "here is the API and how it works" and then these examples should be moved into a "how to" section and fleshed out a bit more. But that's not in scope for this PR where I'm just trying to make sure the examples work at all.

@sabina-talipova
Copy link
Contributor

Really this whole section of docs needs to be reconsidered, probably being split into a more traditional "here is the API and how it works" and then these examples should be moved into a "how to" section and fleshed out a bit more. But that's not in scope for this PR where I'm just trying to make sure the examples work at all.

We should, probably, update this section. Since information presented in ## Using Injector to customise GraphQL queries section is not enough to use it as an example for testing.

Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fix spelling mistake cient to client

@GuySartorelli
Copy link
Member Author

We should, probably, update this section. Since information presented in ## Using Injector to customise GraphQL queries section is not enough to use it as an example for testing.

I've made a note about this in #129, which is about reviewing the structure and general content of front-end related documentation. It falls outside the scope of this PR, which is really just about making the graphql code examples work - not necessarily about making them clear and easy to understand.

In other words, we'll do this in stages:

  • Stage one is getting the code example to work at all. That's this PR
  • Stage two is restructuring the doc so that the code example and API documentation are separate, and independently easy to follow. That's Review all front-end-related documentation #129

For the purposes of testing this PR, I'll send you the project I used to do so with information about how to use it for testing this PR.

@GuySartorelli GuySartorelli force-pushed the pulls/4.13/injectable-graphql-again branch from 1a9525d to dc979b0 Compare April 17, 2023 01:03
@GuySartorelli
Copy link
Member Author

I've fixed the typo.

Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGFM

@sabina-talipova sabina-talipova merged commit a3fae16 into silverstripe:4.13 Apr 17, 2023
@sabina-talipova sabina-talipova deleted the pulls/4.13/injectable-graphql-again branch April 17, 2023 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants