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

Make all GraphQL core queries injectable #611

Closed
3 tasks done
chillu opened this issue Aug 22, 2018 · 6 comments
Closed
3 tasks done

Make all GraphQL core queries injectable #611

chillu opened this issue Aug 22, 2018 · 6 comments

Comments

@chillu
Copy link
Member

chillu commented Aug 22, 2018

Overview

We've done ground work on customiseable GraphQL queries a few months ago. The sole purpose of this was to make the CMS UI extensible. Yet somehow we ended up with AssetAdmin still having hardcoded queries (see readFilesQuery.js and AssetAdmin.js)

Extract from the ACs:

The following use cases are made possible through the API. This can be demonstrated by sample code and recipes rather than full implementations.

  • Show a "pending approval" indicator on a file gallery tile in asset admin

I'm wondering how this was achieved without making the readFiles query injectable?

Acceptance Criteria

Pull Requests

@bergice bergice assigned bergice and unclecheese and unassigned bergice Sep 11, 2018
@bergice
Copy link
Contributor

bergice commented Sep 13, 2018

Discussed this with @unclecheese.

He proposes that we move away from the DI on the front-end and instead move this to the back-end to be composed and presented using FormSchema to the front end as this should make it easier to extend and less abstract, but at the cost of coupling the front-end with the back-end.

The concern is that the way you would extend this using DI on the front-end would be too abstract and not able to scale.

We should look into a quick and clean way to make the asset-admin query extensible based on the requirements of the TNZ project as this should be the current priority.

@unclecheese
Copy link

unclecheese commented Oct 8, 2018

POC is up here, (https://github.com/silverstripeltd/tourism1/tree/feature/injectable-queries) using two different forks -- one for admin/ and one for asset-admin/. There are bigger implications for how this will work that I've discussed with @chillu (out of scope for this card), but these changes get the story over the line.

@unclecheese
Copy link

unclecheese commented Oct 8, 2018

Blocking based on all kinds of stuff up in the air on whether this is the right approach or not. Impossible to call this ready for peer review at this point, but done enough to satisfy edge case.

@robbieaverill
Copy link
Contributor

robbieaverill commented Oct 15, 2018

I've reviewed all PRs and tested it out, all working nicely. Each PR could do with being rebased against 1.3 and have a good squash and re-label. I also expect someone else is going to review this as well =)

@robbieaverill
Copy link
Contributor

All PRs merged

@unclecheese unclecheese removed their assignment Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants