Skip to content

Consolidate use of DOMContentLoaded to wp-dom-ready package#13383

Merged
zinigor merged 2 commits intomasterfrom
update/extensions-use-dom-ready
Sep 3, 2019
Merged

Consolidate use of DOMContentLoaded to wp-dom-ready package#13383
zinigor merged 2 commits intomasterfrom
update/extensions-use-dom-ready

Conversation

@simison
Copy link
Copy Markdown
Member

@simison simison commented Sep 2, 2019

Consolidates all various usages of DOMContentLoaded event to external @wordpress/dom-ready package.

I checked that it's shipped in WordPress 5.1.1 at least, maybe even earlier.

domReady is super simple but it does a couple important checks in case the code runs after DOMContentLoaded event has already fired in case the script loads late. There are various ways to do this by checking readyState (or forgetting to check at all 😉) but like this we can ensure the same pattern everywhere.

This also saves a bit of code by sharing it from a package but that's not very significant. ;-) On the cons side it adds one network request but it'll get concatenated by caching plugins anyway.

Changes proposed in this Pull Request:

  • Change Tiled gallery, Slideshow, Mailchimp and Recurring payments blocks to use wp.domReady. Enqueued dependencies for blocks are handled automatically so this will get loaded without manually adding it to anywhere.
  • Change Photon.js to use wp.domReady and add package to dependencies.
  • Fix previously unguarded use of document.body.addEventListener (document is in fact window.document)

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Refactors existing features

Testing instructions:

Blocks
  • Remember to build before testing (yarn build)
  • Create a post with Tiled gallery, Slideshow, Mailchimp and Recurring payments blocks. For recurring payments block you'll need a site with paid plan.
  • Look at the post from the frontend of the site — do blocks work as expected, are they interactive?
  • Do pages with those blocks enqueue wp-dom-ready script? You can also confirm that _inc/blocks/BLOCK_NAME/view.deps.json file has wp-dom-ready
Photon
  • Enable image CDN/photon
  • Add an image to a post
  • When modules/photon/photon(.min).js is loaded, does restore_dims function run? (maybe add console log to it to confirm?).

Proposed changelog entry for your changes:

@simison simison added [Status] Needs Review This PR is ready for review. [Type] Janitorial [Pri] Low [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Sep 2, 2019
@simison simison added this to the 7.8 milestone Sep 2, 2019
@simison simison requested a review from a team September 2, 2019 08:22
@matticbot
Copy link
Copy Markdown
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32323-code before merging this PR. Thank you!

@jetpackbot
Copy link
Copy Markdown
Collaborator

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against f976768

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 3, 2019
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me. 👍

@zinigor zinigor merged commit d0453da into master Sep 3, 2019
@zinigor zinigor deleted the update/extensions-use-dom-ready branch September 3, 2019 19:57
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 3, 2019
simison added a commit that referenced this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] Low Touches WP.com Files [Type] Janitorial

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants