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

Editor: Ignore save edit in undo history. #17452

Merged
merged 6 commits into from Sep 16, 2019

Conversation

@epiqueras
Copy link
Contributor

commented Sep 16, 2019

Description

This PR fixes the leftover issue from #17259 (comment) by adding functionality to core-data that allows edits to opt out of being tracked in undo history and using that for the edit that persists the latest content right before saving.

It also adds a test to guard for this regression.

How has this been tested?

The steps from #17259 (comment) no longer create an extra undo level.

Types of Changes

New Feature: core-data edits can now opt out of being tracked in history through an extra optional parameter, ignoreUndo.

Bug Fix: Extra undo levels are no longer created when saving the post.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@epiqueras epiqueras added this to the Future milestone Sep 16, 2019
@epiqueras epiqueras self-assigned this Sep 16, 2019
@epiqueras epiqueras force-pushed the fix/ignore-save-edit-in-undo-history branch from e6c2ee3 to e8f75f7 Sep 16, 2019
@epiqueras epiqueras requested review from gziolo and ntwb as code owners Sep 16, 2019
@epiqueras epiqueras referenced this pull request Sep 16, 2019
5 of 5 tasks complete
*
* @return {Object} Action object.
*/
export function* editEntityRecord( kind, name, recordId, edits ) {
export function* editEntityRecord( kind, name, recordId, edits, undoIgnore ) {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 16, 2019

Contributor

Should we mark this argument as experimental? Also maybe an object is better in case we'd need more "options" in the future?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor

I don't think it should be experimental, but it should be an object.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.5 Sep 16, 2019
Copy link
Contributor

left a comment

Tested and works well for me.

Copy link
Member

left a comment

I still have the issue on publish.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Added an e2e test for publishing. Would be great if this could be ignored the same way.

@ellatrix

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

I guess publishing is something that could be undone if it actually reverts the post to a draft. But not sure if that would be an expected user experience.

@@ -155,7 +155,7 @@ export default compose( [
withDispatch( ( dispatch ) => {
const { editPost, savePost } = dispatch( 'core/editor' );
return {
onStatusChange: ( status ) => editPost( { status } ),
onStatusChange: ( status ) => editPost( { status }, { undoIgnore: true } ),

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 16, 2019

Member

I wonder if this is the right place to make this decision. Should status changes in general be ignored? Or publish status changes? Genuinely wondering...

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 16, 2019

Author Contributor

It depends on the context and the entity.

This will cover publishing status changes for posts and it makes sense to ignore that.

It will be very context-dependent which is why I didn't make it part of the entity configuration.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 16, 2019

Member

Ok fair enough.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Good catch @ellatrix! I don't think these or any other post status changes should be part of undo.

Fixed here: c95501a

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

JS unit tests are failing

@epiqueras epiqueras merged commit 06ba5af into master Sep 16, 2019
4 of 7 checks passed
4 of 7 checks passed
pull-request-automation
Details
Header rules - gutenberg-playground No header rules processed
Details
Pages changed - gutenberg-playground All files already uploaded
Details
Redirect rules - gutenberg-playground No redirect rules processed
Details
Mixed content - gutenberg-playground No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
netlify/gutenberg-playground/deploy-preview Deploy preview ready!
Details
@epiqueras epiqueras deleted the fix/ignore-save-edit-in-undo-history branch Sep 16, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* Core Data: Add capability to ignore specific edits in undo history.

* Editor: Ignore save edits in undo history.

* Core Data: Put `undoIgnore` option in an options object.

* Add e2e test for publishing

* Editor: Ignore publish status changes in undo history.

* Editor: Fix unit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.