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

Plugin: Remove jQuery heartbeat-to-hooks proxying #13576

Merged
merged 1 commit into from
Feb 4, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 29, 2019

Previously: #11781

This pull request seeks to remove the jQuery heartbeat event proxying introduced in #11781, which was intended to serve to automatically upgrade heartbeat events to corresponding hooks actions. Due to similar timeframe, it appears the changes from #11781 were made redundant as part of the following core changeset:

https://core.trac.wordpress.org/changeset/43939

Indeed, one can observe through Redux DevTools or a breakpoint in the PostLockedModal's handling of receivePostLock that the heartbeat actions are handled twice in the most recent release of Gutenberg.

Testing instructions:

Repeat testing instructions from #11781, verifying there are no regressions in the behavior of the post locked modal.

Verify that each heartbeat action bound by PostLockedModal is handled only once. You can force a heartbeat connection in your console using the following command:

wp.heartbeat.connectNow()

@aduth aduth added the Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts label Jan 29, 2019
@oandregal
Copy link
Member

With this PR post locked modal works as expected and with these changes UPDATE_POST_LOCK is only fired once. I've also checked that the hooks set up in the core patch are the same that we remove here.

I'd approve this if it wasn't for the existing conflicts in lib/client-assets.php file. If you can take a look and rebase, I can approve later.

@aduth aduth force-pushed the remove/jquery-heartbeat-proxy branch from 88f218c to 408b373 Compare January 31, 2019 18:54
@aduth
Copy link
Member Author

aduth commented Jan 31, 2019

Thanks for looking @nosolosw ! I've pushed a rebased branch to resolve conflicts.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

Checked that it still works after rebasing.

@aduth aduth merged commit 22efd75 into master Feb 4, 2019
@aduth aduth deleted the remove/jquery-heartbeat-proxy branch February 4, 2019 19:13
@youknowriad youknowriad added this to the 5.1 (Gutenberg) milestone Feb 5, 2019
daniloercoli added a commit that referenced this pull request Feb 5, 2019
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Make the modal title styling consistent (#13669)
  Disable navigation block for text mode. (#12185)
  Fix: Linting problem in modal example code (#13671)
  Add myself as a code owner to the annotations (#13672)
  Add more reviewers to CODEOWNERS.md file (#13667)
  Plugin: Remove jQuery heartbeat-to-hooks proxying (#13576)
  Workflow: Add repository CODEOWNERS file (#13604)
  Add a mobile minimum size for form fields (#13639)
  Update edit-save documentation  (#13578)
  Alt image setting (#13631)
  Fix: Allow years lower than 1970 in DateTime component. (#13602)
  Using addQueryArgs to generate Manage All Reusable Blocks link (#13653)
  Bump plugin version to 5.0.0-rc.1 (#13652)
  Update lodash to 4.17.10 (#13651)
  Refreshed PR (#9469)
  Set default values of the width and height input fields according to the actual image dimensions (#7687)
  12647 fix css color picker (#12747)
  Remove "we" from messages (#13644)
  Fix: Font size picker max width on mobile (#13264)
  Fix/issue 12501 menu item aria label
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants