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

History viewer on other modeladmins #62

Open
sminnee opened this issue Sep 14, 2018 · 2 comments
Open

History viewer on other modeladmins #62

sminnee opened this issue Sep 14, 2018 · 2 comments

Comments

@sminnee
Copy link
Member

sminnee commented Sep 14, 2018

Original description

Currently, the history viewer requires a lot of per-datatype boilerplate, which makes it difficult to add to other modules.

"You need to add graphql queries as well, see registerComponents.js and client/src/state/history/* in the CMS module for an example. Dynamic graphql queries werent an option when we wrote it"

If we have better support for dynamic queries, we should reduce the amount of boilerplate required, so that we can add HistoryViewer to a different datatype ideally without requiring custom JS.

Clear problematic

  • There is a lot of boilerplate required to add a history tab to arbitrary models
  • Only one model can have a history viewer at a time due to the lack of specificity in the HistoryViewer.Form_ItemEditForm scope used when injecting the history viewer to the DOM
@sminnee sminnee added type/enhancement New feature or request affects/v4 labels Sep 14, 2018
@robbieaverill
Copy link
Contributor

Yeah for sure. At the moment the requirements are:

  • Add the HistoryViewerField to your DataObject's CMS fields
  • Define GraphQL scaffolding to expose the necessary DB fields
  • Add a GraphQL query (Javascript) and apply it to the HistoryViewer component with JS Injector transformation

If we were able to use query fragments dynamically then we could possibly eliminate a the need for the JS GraphQL query, but we'd still need to have the user opt in to exposing their data in the API I guess. I'm not sure we could do that dynamically...

There'd be a couple of blockers on this - two I know of off hand are silverstripe/silverstripe-graphql#176 and silverstripe/silverstripe-graphql#182 which I think will allow us to use fragments that refer to subclasses of the base query's object type - at the moment you can't do that so fragments have limited value (statement is while using basic YAML scaffolding).

@GuySartorelli
Copy link
Member

There are two parts to this problem, and I've updated the issue description to list them clearly. There's no sense reducing boilerplate for history viewer when the injector scope still only allows one model to have a history viewer per project.

We can solve the scope issue by having GridFieldDetailForm_ItemRequest add the model class as a data attribute to the form, and then use that additional context when injecting the history viewer:

const cmsContent = this.closest('.cms-content').attr('id');
const context = (cmsContent)
? { context: cmsContent }
: {};
const HistoryViewerComponent = loadComponent('HistoryViewer', context);

We'd have to figure out which class name to specify though - is it the base class (e.g. BaseElement)? Or the class of the specific instance (e.g. ElementContent)? Or the whole inheritance chain (e.g. [BaseElement, ElementContent])?

nb; this assumes that what is being discussed above doesn't magically solve this problem - I don't understand graphql well enough to know if it does or not.

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

3 participants