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

Enable post locking in Gutenberg #4217

Merged
merged 119 commits into from
Oct 4, 2018
Merged

Enable post locking in Gutenberg #4217

merged 119 commits into from
Oct 4, 2018

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Jan 1, 2018

Description

Enable the post locking feature in gutenberg:

Fixes #4331

  • uses the existing mechanism to set the post as locked.
  • works in both and between classic/gutenberg editors.
  • uses the existing core code, slightly refactored to pass Gutenberg linting.
  • show the locked dialog when opening a post that is already opened by another user.
  • show the takeover dialog shown when someone tries to take over a locked post.

How Has This Been Tested?

  • Create or edit a post.
  • Log in as a separate user and go to the posts screen, note the post is locked.
  • Test the three buttons: preview, take over and post list
  • Take over the post, make some edits without saving, then toggle back to the browser where the other user had the post open, this user will get an autosave fired and the 'another user has take over' dialog.

Screenshots (jpeg or gifs if applicable):


Types of changes

  • Add heartbeat.js in editor/utils, exporting setupHearthbeat
  • call setupHearthbeat before initial editor render.
  • monitor heartbeat for lock events - displaying a "user has taken over" modal when another user takes over the post
  • localize data for existing locks, displaying an "Already Locked" modal

Checklist:

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

@adamsilverstein adamsilverstein mentioned this pull request Jan 1, 2018
3 tasks
@adamsilverstein
Copy link
Member Author

Closing in favor of #4218 which builds on this and adds heartbeat based autosaves as well.

@gziolo gziolo deleted the feature/post-locking branch May 7, 2018 11:15
@adamsilverstein adamsilverstein restored the feature/post-locking branch June 8, 2018 16:35
@adamsilverstein
Copy link
Member Author

Reopening to work on standalone post locking since we took a different route with autosaves.

@adamsilverstein adamsilverstein self-assigned this Jul 27, 2018
@mtias mtias modified the milestones: Merge: Editor, 4.1 Oct 3, 2018
@youknowriad youknowriad modified the milestones: 4.1, 4.0 Oct 4, 2018
gutenberg.php Outdated Show resolved Hide resolved
lib/client-assets.php Show resolved Hide resolved
lib/client-assets.php Show resolved Hide resolved
packages/components/src/modal/README.md Outdated Show resolved Hide resolved
/**
* External dependencies
*/
import jQuery from 'jquery';
Copy link
Member

Choose a reason for hiding this comment

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

We should add jquery as a dependency of the wp-editor script

This still needs to be done.

packages/editor/src/components/post-locked-modal/index.js Outdated Show resolved Hide resolved
updatePostLock,
};
} ),

Copy link
Member

Choose a reason for hiding this comment

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

We're inconsistent with newlines between these members of the array. I'd recommend omitting them for broader consistency with the project.

packages/editor/src/store/reducer.js Show resolved Hide resolved
packages/editor/src/store/reducer.js Show resolved Hide resolved
@youknowriad youknowriad force-pushed the feature/post-locking branch from 1e140a0 to 240f3e7 Compare October 4, 2018 15:53
@karmatosed
Copy link
Member

Looks good to me. Thanks for all the hard work.

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Added i18n feedback, which I'll address right now. (By typing it out I can refer to it later.)

packages/editor/src/components/post-locked-modal/index.js Outdated Show resolved Hide resolved
packages/editor/src/components/post-locked-modal/index.js Outdated Show resolved Hide resolved
@youknowriad youknowriad merged commit 37bd1e6 into master Oct 4, 2018
@youknowriad youknowriad deleted the feature/post-locking branch October 4, 2018 16:50
@youknowriad
Copy link
Contributor

Thanks for the hard work everyone. Now it's in we can enhance it with API endpoints at a more reasonable pace :)

@aduth
Copy link
Member

aduth commented Oct 4, 2018

Now it's in we can enhance it with API endpoints at a more reasonable pace :)

Is this tracked as an issue somewhere I can follow?

@danielbachhuber
Copy link
Member

Is this tracked as an issue somewhere I can follow?

There's https://core.trac.wordpress.org/ticket/44862

// https://developer.wordpress.org/plugins/javascript/heartbeat-api/
jQuery( document )
.on( 'heartbeat-send.refresh-lock', this.sendPostLock )
.on( 'heartbeat-tick.refresh-lock', this.receivePostLock );
Copy link
Member

Choose a reason for hiding this comment

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

@adamsilverstein is there a reason we need to rely on jQuery here? could we have gotten by with addEventListener() (and for IE attachEvent)?

given the assumption that WordPress is already loaded jQuery seems free, but for external uses these few lines can pull in a huge dependency

thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

We should do similar as to what was done in #8311 as a short-term solution.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, at least for the heartbeat-tick event, we should already have the action provided through #8311.

Copy link
Member

Choose a reason for hiding this comment

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

Pull request at #11781

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.