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

NEW Implement React history viewer if available (SS 4.2 onwards) #207

Merged

Conversation

robbieaverill
Copy link
Contributor

Requires SS 4.2 or higher, so will need to be added to a new minor release.

Parent story: silverstripe/silverstripe-versioned#37

@raissanorth
Copy link
Contributor

It appears that actions are listed more than once. I have done the following:

  1. I added a new content block.
  2. I published it.
  3. I made changes, then saved it.
  4. I published it.

This is the result I see:

image

@robbieaverill
Copy link
Contributor Author

Do you see the same thing in the current history viewer for blocks, i.e. elemental 2.0 on SS 4.0 or 4.1?

If there are duplicate versions in list it's likely to be coming from the ORM, since this component is essentially a wrapper around the versions that come for a record in GraphQL, which is also just listing versions from the silverstripe/versioned module - I'd suspect there's a logical reason why it displays like this, although I agree that it might be confusing for users

@raissanorth
Copy link
Contributor

When I test this there is a bit of a delay in the changes showing up in the History Viewer.

  1. I add a new content block.
  2. I save that content block.

I then move to the History tab and see:

image

I refresh the page. This opens the Content tab.

I open the History tab and see:

image

Also, am I right in the assumption that the three items listed are: 1) Block creation, 2) Block saving, and 3) Block publishing?!

@robbieaverill
Copy link
Contributor Author

Also, am I right in the assumption that the three items listed are: 1) Block creation, 2) Block saving, and 3) Block publishing?!

Yep


I think the state of the results not updating needs to be addressed in the history viewer itself, in that we should hook into the save and publish actions to update the history viewer's state or cause it to refetch its records - let's wait until it gets merged in some state then raise an issue for that

@raissanorth
Copy link
Contributor

Do you see the same thing in the current history viewer for blocks, i.e. elemental 2.0 on SS 4.0 or 4.1?

I repeated the same steps and actually see the same thing happening on SS 4.2.x-dev and Elemental 2.1.x-dev:

image

@clarkepaul
Copy link

Just to recap on our discussion today. With the current functionality we can change the wording "Modified" back to "Saved" so that it is following the pattern of which action was pressed rather than the state at the time (hopefully thats easier).

It would be good to hear if anyone has preferences for using the States here (Draft/Published/Modified) instead of the completed action (Saved/Published). If possible we'd avoid combining them as "Saved as modified" doesn't make sense.

@robbieaverill
Copy link
Contributor Author

If at some stage we want to show draft, modified and published as separate labels then we might need to consider adding some API to GraphQL that allows for that now - currently we have Published: boolean only and use that to determine the label. We could also add some React logic to return Draft before the record has first been published, but it might be better for this logic to be contained to the versioned API over a React implementation of it

@robbieaverill
Copy link
Contributor Author

- Modified
+ Saved

Changed in silverstripe/silverstripe-versioned-admin#2

// the query must be set to the "versions" prop on the component that this HOC is
// applied to (see ElementHistoryViewer.js) for binding implementation.
const query = gql`
query readOneBlock ($block_id: ID!, $limit: Int!, $offset: Int!) {

Choose a reason for hiding this comment

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

This query could also have a more distinct name.

loading: networkLoading || !versions,
versions,
graphQLErrors: errors,
actions: Object.assign({}, actions, {

Choose a reason for hiding this comment

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

Object spreads, please. :)

const withElementsHistoryViewer = (HistoryViewer) => compose(
readOneBlockQuery
)(HistoryViewer);

Choose a reason for hiding this comment

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

I might be missing something, but I don't think this function is necessary. readOneBlockQuery exports an HOC function, expecting a Component as an argument. This should suffice:

updater.component(
  'HistoryViewer.Form_ItemEditForm',
  readOneBlockQuery,
  'ElementHistoryViewer'
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right

Copy link

@unclecheese unclecheese left a comment

Choose a reason for hiding this comment

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

Looks good. Biggest thing is what appears to be a superfluous HOC definition.

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Mar 26, 2018

Todo:

  • Remove HOC definition
  • Update query name to be less generic
  • Use object spread operator in GraphQL query
  • Replace GraphQL fields wildcard with a more specific subset

@robbieaverill robbieaverill force-pushed the pulls/2.1/history-viewer branch from ac2bf67 to 7c1a649 Compare March 27, 2018 01:07
@robbieaverill
Copy link
Contributor Author

@raissanorth mind having a quick once over this pull request?

Copy link
Contributor

@raissanorth raissanorth left a comment

Choose a reason for hiding this comment

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

Haven't had a chance to run this locally yet, but the code itself looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants