-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
editor/effects.js
Outdated
} ); | ||
dispatch( { | ||
type: 'UPDATE_POST', | ||
edits: newPost, |
There was a problem hiding this comment.
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 😄 )
There was a problem hiding this comment.
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.
editor/effects.js
Outdated
new wp.api.models.Post( toSend ).save().done( ( newPost ) => { | ||
dispatch( { | ||
type: 'REQUEST_POST_UPDATE_SUCCESS', | ||
post: newPost, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that. I'm still not used to the actions chaining, I only check the reducers :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
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.)