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

Only save metaboxes when it's not an autosave #7502

Merged
merged 2 commits into from Jun 25, 2018

Conversation

Projects
None yet
4 participants
@danielbachhuber
Member

danielbachhuber commented Jun 22, 2018

Description

Improves autosave behavior to disable accidental saving of metaboxes. Metaboxes should only be saved on full saves.

See conversation in https://wordpress.slack.com/archives/C02QB2JS7/p1529698040000346

Fixes #7135

How has this been tested?

  1. Open a new post in Gutenberg. Make sure your editor includes a metabox.
  2. Type some text and wait for an autosave.
  3. Type additional text and wait for a second autosave.
  4. Hit "Save Draft" a first time.
  5. Hit "Save Draft" a second time.

When the revision UI appears in Post Settings after step 5, it should indicate 2 revisions, not 4:

image

Previously, the two autosave events would trigger full save events / revision creation.

Screenshots

autosaving

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@danielbachhuber danielbachhuber added this to the 3.2 milestone Jun 22, 2018

@danielbachhuber danielbachhuber requested review from WordPress/gutenberg-core and adamsilverstein Jun 22, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 25, 2018

Thanks for working on this.

Interesting question pops up for me — if we were to want to look at an autosave overhaul in a distant future version of Gutenberg, one where there is no button at all, there's just an autosave that kicks in when a change is detected, we probably wouldn't be able to make that work for metaboxes. In such a future, could we make the button appear explicitly only when metaboxes are present?

@@ -54,10 +54,21 @@ const effects = {
store.dispatch( setMetaBoxSavedData( dataPerLocation ) );
// Saving metaboxes when saving posts
const { isAutosavingPost } = select( 'core/editor' );
let shouldRequestMetaBoxUpdates = false;
subscribe( onChangeListener( select( 'core/editor' ).isSavingPost, ( isSavingPost ) => {

This comment has been minimized.

@youknowriad

youknowriad Jun 25, 2018

Contributor

I think it's not correct at 100% to use the onChangeListener to call the listener while also watching for changes in the isAutosavingPost selector. Maybe we could add support for watching multiple selectors in onChangeListener or just drop it and use custom logic.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 25, 2018

Member

Maybe we could add support for watching multiple selectors in onChangeListener or just drop it and use custom logic.

What custom logic would you use?

This comment has been minimized.

@youknowriad

youknowriad Jun 25, 2018

Contributor

Just checking both selector results without extracting the logic in a utility like onChangeListener

let wasSavingPost = select( 'core/editor' ).isSavingPost();
let wasAutosavingPost = select( 'core/editor' ). isAutosavingPost();
subscribe( () => {
  const isSavingPost = select( 'core/editor' ).isSavingPost();
  const isAutosavingPost = select( 'core/editor' ).isSavingPost();

  const shouldTriggerMetaboxesSave = wasSavingPost && ! wasAutosavingPost && ! isSavingPost && !isAutosavingPost; // not certain about this check entirely

  // Restore 
  wasSavingPost = isSavingPost;
  wasAutosavingPost = isAutosavingPost;

  // trigger
  if ( shouldTriggerMetaboxesSave ) {
    // do something.
  }
} ) 

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 25, 2018

Member

Do you think this custom logic is better than what I have? At first glance, what I have already seems more precise.

This comment has been minimized.

@youknowriad

youknowriad Jun 25, 2018

Contributor

It's not about better or worse, it's about the fact that we want to run the listener whenever the value of isAutosavingPost's selector changes. In your code it's not the case, if for some reason (even if it's not possible at the moment) isSavingPost doesn't change but isAutosavingPost does, your listener is not being executed and I don't think it's intentional.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 25, 2018

Member

subscribe( () => { fires for every change, which seems too frequent. Is there an alternative you'd suggest?

This comment has been minimized.

@youknowriad

youknowriad Jun 25, 2018

Contributor

There is none actually, we can bundle the multiple selectors support into onChangeListener. Maybe by supporting an "array of selectors" instead of a given "selector" as an argument passed to onChangeListener but in terms of performance, it doesn't change anything compared to my logic above.

That's how redux work in general, a unique eventEmitter calling all its listeners, It has proved to scale well in several contexts.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 25, 2018

Member

Ok, updated in ed79968

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jun 25, 2018

@youknowriad

LGTM 👍 Thanks for the updates

@aduth

This comment has been minimized.

Member

aduth commented Jun 25, 2018

Some previous context / discussion at #6257 (comment)

Open questions:

  • What's the relationship between autosave and saving meta box values? This has not been changed from its previous logic, where metabox updates trigger in response to the start of a save request (including autosave).
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 25, 2018

@aduth Does your comment block merge?

@aduth

This comment has been minimized.

Member

aduth commented Jun 25, 2018

Nope.

@danielbachhuber danielbachhuber merged commit d7dc8f2 into master Jun 25, 2018

2 checks passed

codecov/project 46.76% (-0.04%) compared to f143608
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the 7135-limited-autosave branch Jun 25, 2018

oxyc added a commit to generoi/gutenberg that referenced this pull request Jun 26, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg
* 'master' of https://github.com/WordPress/gutenberg: (69 commits)
  fix: Show permalink editor in editor (WordPress#7494)
  Fix text wrapping in Firefox. (WordPress#7472)
  Try another approach to fixing the sibling inserter in Firefox (WordPress#7530)
  fix: Improve "add block" text in NUX onboarding (WordPress#7511)
  Implement core style of including revisions data on Post response (WordPress#7495)
  Testing: Add e2e test for PluginPostStatusInfo (WordPress#7284)
  Add end 2 end test for sidebar behaviours on mobile and desktop. (WordPress#6877)
  Only save metaboxes when it's not an autosave (WordPress#7502)
  Fix broken links in documentation (WordPress#7532)
  Remove post type 'viewable' compatibility shim (WordPress#7496)
  Fix typo. (WordPress#7528)
  Blocks: Remove wrapping div from paragraph block (WordPress#7477)
  fix: change import for InnerBlocks (WordPress#7484)
  Polish library just a teeeeensy bit (WordPress#7522)
  feat: Add snapshot update script (WordPress#7514)
  Display server error message when one exists (WordPress#7434)
  Fix issues with gallery in IE11. (WordPress#7465)
  Polish region focus style (WordPress#7459)
  Fix IE11 formatting toolbar visibility (WordPress#7413)
  Update plugin version to 3.1. (WordPress#7402)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment