Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Jun 7, 2017

Related: #945 (comment)

This pull request seeks to defer save payload to be generated during the save effect handler, instead of making it the responsibility of each saving component to construct. Instead, the component can merely call savePost. There's a bit of uncomfortable magic in allowing the effect handler to make heavy use of the current state, but arguably outweighed by the usability benefit. I'd still like to consider further splitting the REQUEST_POST_UPDATE handler, e.g. standalone handlers for the actual network request, clearing edits.

Testing instructions:

Ensure unit tests pass:

npm test

(Will need to revisit #966 with updated effect tests)

Verify that there are no regressions in: saving, publishing, privately published, or changing status of an existing or new post and updating / saving said post.

@aduth aduth requested a review from youknowriad June 7, 2017 17:45
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

}

export function savePost( postId, edits ) {
export function savePost() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to provide a way to explicitly pass the edits we want to persist. I'm wondering if the "Revert to draft" button should save all the current edits or only the status (could be resolved in the "Revert to draft" PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

An interesting question. Another question is whether we want the "Revert to draft" action to immediately request the update, or wait until an explicit Save/Update action. The latter is the behavior of the current editor, but per the mockup shown at #945 (comment) , the expectation with a button might be more that it would take effect immediately.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, yes, let's defer this to the "Revert to draft" changes.

@aduth aduth merged commit 7c990c2 into master Jun 7, 2017
@aduth aduth deleted the add/save-middleware branch June 7, 2017 19:11
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.

3 participants