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

Fixed: Map js loading everywhere #161

Merged
merged 1 commit into from Oct 10, 2023
Merged

Fixed: Map js loading everywhere #161

merged 1 commit into from Oct 10, 2023

Conversation

jayedul
Copy link
Contributor

@jayedul jayedul commented Feb 14, 2023

Description of the Change

Map js library was loading everywhere. Now it is fixed.

Closes #147

How to test the Change

  • Create a page or post
  • Add Apple Map block
  • Save and visit
  • Open network log and make sure https://cdn.apple-mapkit.com/mk/5.x.x/mapkit.js is loading
  • Now edit the post, remove the map block, save and visit again
  • Now you should see the library is not loading anymore. Previously it used load even though there is no map block in the post.

Changelog Entry

Fixed - Mapkit js loading everywhere in frontend despite no Map block there is

Credits

Props @jayedul

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@jayedul jayedul requested review from a team and fabiankaegy as code owners February 14, 2023 13:22
@jayedul jayedul requested review from iamdharmesh and removed request for a team February 14, 2023 13:22
@jayedul jayedul self-assigned this Feb 14, 2023
Copy link
Member

@fabiankaegy fabiankaegy left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution :)

The change currently only adds the apple-mapkit-js dependencie to the editor script of the block. However it also needs to be present in the script we load on the frontend via the viewScript handle: maps-block-apple-frontend

However I still don't quite understand why the script is loading everywhere. As per the documentation any WPDefinedAsset loaded via the script key should only be enqueued in the editor and on the frontend if the block is actually being used. https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#script

@jayedul
Copy link
Contributor Author

jayedul commented Feb 14, 2023

apple-mapkit-js is already used as dependency for frontend.
s

However, I'm also unsure why the file loads everywhere when it was registered with script key.

@jeffpaul jeffpaul added this to the 1.1.1 milestone Feb 22, 2023
@dkotter dkotter modified the milestones: 1.1.1, 1.2.0 Jun 21, 2023
@jeffpaul
Copy link
Member

jeffpaul commented Sep 1, 2023

@fabiankaegy @jayedul is there still work needed here or can this move along in review and towards merge?

@jeffpaul
Copy link
Member

@10up/open-source-practice would be good if someone could help get this through review/merge... thanks!

Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

LGTM

Working as expected:

image

IMO, no more work is required; looks good to merge.

@jeffpaul jeffpaul merged commit ebbb856 into develop Oct 10, 2023
@jeffpaul jeffpaul deleted the fix/147 branch October 10, 2023 13:54
@dkotter dkotter modified the milestones: 1.2.0, 1.1.2 Oct 13, 2023
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.

MapKit JS loads on every page, regardless of plugin usage
5 participants