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

State: Optimistically update the saved post #1093

Merged
merged 3 commits into from
Jun 12, 2017
Merged

Conversation

youknowriad
Copy link
Contributor

alternative to #1092

Two things:

  • I'm using redux-optimist to optimistically save the current post when we trigger the request and revert in case of failure.

  • I'm adding a UPDATE_POST action to separate network actions from data actions (which could allow offline mode.)

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jun 9, 2017
@youknowriad youknowriad self-assigned this Jun 9, 2017
@youknowriad youknowriad requested a review from aduth June 9, 2017 15:36
} );
dispatch( {
type: 'UPDATE_POST',
edits: newPost,
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like something we should merge into our representation of a post, per the behavior of the reducer, but instead reset / replace with. Maybe a separate RESET_POST that has the same behavior as the currentPost handler for RESET_BLOCKS? In fact, I'm thinking maybe we should separate out a RESET_POST from RESET_BLOCKS anyways, because the post property here feels kinda awkwardly included anyways:

https://github.com/WordPress/gutenberg/blob/6dea0df/editor/index.js#L49-L53

Maybe instead:

store.dispatch( { type: 'RESET_POST', post } );
store.dispatch( { type: 'RESET_BLOCKS', blocks: wp.blocks.parse( post.content.raw ) } );

Or maybe we don't need RESET_BLOCKS at all, and consolidate further to a single RESET_POST which the block reducers listen for and parse upon.

(Forgive my spew of brain-thoughts 😄 )

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 the "block" state is more close to "edits" than it is to "currentPost". Thus, we shouldn't reset its value after a save success, the user could have updated it.

new wp.api.models.Post( toSend ).save().done( ( newPost ) => {
dispatch( {
type: 'REQUEST_POST_UPDATE_SUCCESS',
post: newPost,
Copy link
Member

Choose a reason for hiding this comment

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

Removing this is causing an error when save completes:

image

aduth
aduth previously requested changes Jun 12, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

@youknowriad youknowriad dismissed aduth’s stale review June 12, 2017 14:09

Thanks for catching that. I'm still not used to the actions chaining, I only check the reducers :)

Copy link
Member

@aduth aduth 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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants