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

fix(class-newspack-blocks.php): assets meta file path for scripts #780

Merged
merged 1 commit into from
Jul 19, 2021

Conversation

divyarajmasani
Copy link
Contributor

Due to incorrect assets path, view and editor scripts were loading fallback dependencies. This
resulted in unnecessary scripts being added to the front end as well as wrong dependencies being
added for editor scripts.

All Submissions:

Changes proposed in this Pull Request:

Closes #779.

How to test the changes in this Pull Request:

Test 1:

  1. Create a new page/post with Homepage Articles block.
  2. Preview or view the page using devTools inspect mode.
  3. Observe that there are no extraneous scripts added at bottom of the page.

Test 2: ( Using debug methods )

  1. If you are using xdebug add a breakpoint at following line
    $script_data['script_path'] = plugins_url( $script_path, NEWSPACK_BLOCKS__PLUGIN_FILE );
  2. Check that $script_data['dependencies'] has the values defined in corresponding dist/{script-path}.assets.php

Due to incorrect assets path, view and editor scripts were loading fallback dependencies. This
resulted in unnecessary scripts being added to the front end as well as wrong dependencies being
added for editor scripts.

fix Automattic#779
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Nice catch, @divyarajmasani! Thanks for the patch. I can confirm that the asset path looks correct now and that the blocks still function as expected on the front-end.

@divyarajmasani
Copy link
Contributor Author

Thank you for verifying @dkoo, Do you have any visibility on when will this be merged?

@dkoo
Copy link
Contributor

dkoo commented Jul 19, 2021

@divyarajmasani We'll merge this for the next release. Thanks again!

@dkoo dkoo merged commit a37ff23 into Automattic:master Jul 19, 2021
matticbot pushed a commit that referenced this pull request Jul 19, 2021
# [1.30.0-alpha.1](v1.29.2...v1.30.0-alpha.1) (2021-07-19)

### Bug Fixes

* **class-newspack-blocks.php:** fixes assets meta file path for scripts ([#780](#780)) ([a37ff23](a37ff23)), closes [#779](#779)
* allow swiper to reinitialize carousel on attribute changes ([#807](#807)) ([cc9fa30](cc9fa30))
* do not initialize swiper instances if component is hidden ([#804](#804)) ([fe599e1](fe599e1))
* **carousel:** preview performance ([#803](#803)) ([e5fc989](e5fc989))

### Features

* **donate:** streamlined block option w/ Stripe ([#784](#784)) ([10bfb0b](10bfb0b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.30.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jul 19, 2021
# [1.30.0](v1.29.2...v1.30.0) (2021-07-19)

### Bug Fixes

* remove autoload dependency from donation class ([#811](#811)) ([c37c885](c37c885))
* **class-newspack-blocks.php:** fixes assets meta file path for scripts ([#780](#780)) ([a37ff23](a37ff23)), closes [#779](#779)
* allow swiper to reinitialize carousel on attribute changes ([#807](#807)) ([cc9fa30](cc9fa30))
* do not initialize swiper instances if component is hidden ([#804](#804)) ([fe599e1](fe599e1))
* **carousel:** preview performance ([#803](#803)) ([e5fc989](e5fc989))

### Features

* **donate:** streamlined block option w/ Stripe ([#784](#784)) ([10bfb0b](10bfb0b))
* **nrh:** donations handling ([#805](#805)) ([821f6db](821f6db))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.30.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Extra scripts being added to the pages where newspack-blocks have been used.
3 participants