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

Add a post autosave lock #16249

Merged
merged 19 commits into from
Sep 5, 2019
Merged

Add a post autosave lock #16249

merged 19 commits into from
Sep 5, 2019

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Jun 23, 2019

Description

Add a post autosave lock, similar to the post saving lock introduced in #10649. When post autosaving is lock, autosaves will not fire. Note that because autosaves are required for previewing, post previews will also be unavailable when post autosaving is locked.

Use case

In working on developing a block based revision display for Gutenberg, I am temporarily modifying block attributes to track and display changes. I need to prevent autosaves at this point to avoid saving the temporary modifications.

How has this been tested/How to test?

Load gutenberg and make some edits. In the console, type:
wp.data.dispatch( 'core/editor' ).lockPostAutosaving( 'mylock' );
Note: the post will no longer autosave.

Remove the post lock with:
wp.data.dispatch( 'core/editor' ).unlockPostAutosaving( 'mylock' );

Test adding multiple locks: autosaving will remain disabled until all locks are removed.

Types of changes

  • Add actions for lock and unlock post autosaving.
  • Add an isPostAutosavingLocked selector.
  • Check isPostAutosavingLocked before autosaving.

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.

@adamsilverstein adamsilverstein force-pushed the feature/autosave-lock branch from 88956d3 to e49367a Compare June 23, 2019 14:41
@adamsilverstein
Copy link
Member Author

Documented usage in docs/designers-developers/developers/data/data-core-editor.md do I need to add documentation elsewhere?

@gziolo
Copy link
Member

gziolo commented Jun 24, 2019

Documented usage in docs/designers-developers/developers/data/data-core-editor.md do I need to add documentation elsewhere?

I'm not aware of any other place where we list such functionalities. However, it looks like we might need tutorial which explains how to have a more grained control over post publishing with custom actions like the one you propose here.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Unit tests and e2e test are failing:

https://travis-ci.com/WordPress/gutenberg/jobs/210262659#L5610

with a quite similar message:

FAIL packages/editor/src/store/test/selectors.js (9.257s)
  ● selectors › isEditedPostAutosaveable › should return true if there is no autosave
    TypeError: Cannot read property 'postAutosavingLock' of undefined```

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Jun 25, 2019

However, it looks like we might need tutorial which explains how to have a more grained control over post publishing with custom actions like the one you propose here.

I will start working on a tutorial, can you suggest which file I should add this to? Also, when I commit this locally, husky complains:

✖ node ./bin/update-readmes found some errors. Please fix them and try committing again.

block-editor
SyntaxError: Unexpected token >

I assume this is because I haven't added or changed a readme. Should that check include other .md files, or do I need to update a README (which one)? or should I instead add --no-verify to make the commit (which is what I did so far)?

@adamsilverstein adamsilverstein requested a review from gziolo June 25, 2019 08:30
@gziolo
Copy link
Member

gziolo commented Jun 27, 2019

I will start working on a tutorial, can you suggest which file I should add this to? Also, when I commit this locally, husky complains:

✖ node ./bin/update-readmes found some errors. Please fix them and try committing again.

block-editor
SyntaxError: Unexpected token >

@nosolosw any ideas on why this fails?

I will start working on a tutorial, can you suggest which file I should add this to?

This is where we keep all the tutorials:
https://github.com/WordPress/gutenberg/tree/master/docs/designers-developers/developers/tutorials

It seems like we would need a new item there. @nosolosw, @mkaz and @chrisvanpatten should have some good ideas as they shaped together most of our docs :)

@gziolo gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement. labels Jun 27, 2019
@gziolo
Copy link
Member

gziolo commented Jun 27, 2019

In working on developing a block based revision display for Gutenberg, I am temporarily modifying block attributes to track and display changes. I need to prevent autosaves at this point to avoid saving the temporary modifications.

I'm thinking now that it might be confusing to have both lockPostAutosaving and lockPostSaving. Do you have some thoughts about how this should work together? I have no idea what use cases are on the table. Do you see a case where autosaving should be locked but it's still fine to allow saving?

@adamsilverstein
Copy link
Member Author

I'm thinking now that it might be confusing to have both lockPostAutosaving and lockPostSaving. Do you have some thoughts about how this should work together? I have no idea what use cases are on the table. Do you see a case where autosaving should be locked but it's still fine to allow saving?

For my particular use case it might not be necessary to separate them - another approach would be to have the postSavingLock ALSO prevent post autosaving. One potential issue with that is that because previews rely on autosaves, locking them means preview is unavailable which might not be what you want when you are locking saving.

Typical use case for save locking it to prevent saving until some pre-flight condition has been met: for example, the content must contain at least one image block, or all images in the content must include alt tags.

My use case for autosave locking is for my (experimental) block revisioning plugin which enables browsing thru revision history to see which blocks were added, which blocks were removed and which were changed.

The plugin currently uses the main editor, loading its revision block display when you enter the revision display mode, and then restoring the post to its original content when you leave the revisions mode. As the post content is temporarily changed, I don't want to save this temporary state in any way.

Some alternate approaches I have considered but haven't explored thoroughly:

  • Blocking autosaving on the server side - sub optimal as the UI will still appear to run autosaves.
  • Setting the autosave interval to a very high time: does not appear to be possible to alter the interval on the fly?
  • Use an overlay copy of Gutenberg to display the temporary block revision view: more complex, likely more resource intensive.

@adamsilverstein
Copy link
Member Author

@gziolo this is ready for additional review/feedback whenever you have some time.

@oandregal
Copy link
Member

@adamsilverstein is #16249 (comment) still happening (the SyntaxError: Unexpected token > error)?

My first thought is that it is related to the introduction of React Fragment syntax some months ago (<>) that I fixed in ed630e8. Can you try rebasing master?

@adamsilverstein
Copy link
Member Author

Can you try rebasing master?

Yep, thanks.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

We still miss actions which enable and disable lock for autosave. We mostly need them to have a way to expose them in docs and make them easier to use. We have similar actions for regular post saving lock: lockPostSaving & unlockPostSaving.

} = select( 'core/editor' );

const { autosaveInterval } = select( 'core/editor' ).getEditorSettings();

return {
isDirty: isEditedPostDirty(),
isAutosaveable: isEditedPostAutosaveable(),
isAutosaveable: isEditedPostAutosaveable() && ! isPostAutosavingLocked(),
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about moving the part which checks whether there is a lock for autosave ! isPostAutosavingLocked() to the isEditedPostAutosaveable selector? It seems like the post can be autosaveable only when there is no lock for autosave.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give that a try

Copy link
Member Author

Choose a reason for hiding this comment

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

packages/editor/src/store/selectors.js Outdated Show resolved Hide resolved
packages/editor/src/components/autosave-monitor/index.js Outdated Show resolved Hide resolved
packages/editor/src/store/test/selectors.js Show resolved Hide resolved
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
@adamsilverstein adamsilverstein requested a review from gziolo August 27, 2019 18:33
@adamsilverstein
Copy link
Member Author

@gziolo This is ready for your re-review.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

All good, thanks for all iterations added 👍

@gziolo
Copy link
Member

gziolo commented Aug 30, 2019

This branch needs to be rebased now, I should merge after review to avoid it 🙃

@adamsilverstein adamsilverstein merged commit 1eca362 into master Sep 5, 2019
@adamsilverstein adamsilverstein deleted the feature/autosave-lock branch September 5, 2019 21:36
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
@youknowriad
Copy link
Contributor

Question here: why not just lock auto saves and preview when saving is locked as well? (to avoid a new API). I feel these might be related but maybe I'm missing something?

@adamsilverstein
Copy link
Member Author

These are related, but different feature to lock/unlock.

Post save locking is useful for controlling updates or publication - for example a pre-flight check. in this case, you still want autosaves to work so a user does not loose their work

A use case where locking autosaves is useful when you want to avoid temporary saves for some reason, for example if you are showing temporary/staged changes in the editor and you don't want these saved. In this case, you might disable saving as well, or keep it enabled but revert temporary changes before saving. Alos, some users may want disable autosaves entirely, this lock enables that with preventing regular saves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants