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

Move embed scripts into the body in preview documents #10434

Merged
merged 2 commits into from
Oct 9, 2018

Conversation

notnownikki
Copy link
Member

Description

Jetpack supplies embeds that use shortcodes, and enqueue scripts needed for the embeds to work. We were including these scripts in <head> but that doesn't work for all embeds. This change moves them into the body of the preview document, which fixes issues with Instagram embeds.

How has this been tested?

Install Jetpack and make sure the shortcode module is enabled.

In a new post, embed https://www.pinterest.co.uk/pin/373869206564305678/ and https://www.instagram.com/p/BoEH8K_lCJ4/?taken-by=juicedpixels

Both should show a preview of the embed.

Types of changes

Bug fix

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I hate to be that person, but is there any chance this could be something we have an E2E test for? I'm not sure how easy it'd be to test the embed appearing or the enabling of the Jetpack plugin without an account. I guess it might be a pain, but this seems a good candidate for something we could easily regress in the future.

Code makes sense to me though. If a test is too much of a pain, maybe you could add a comment about the reason the code is in the body and not the head? I could imagine someone in the future thinking they're clever and moving it back out! 😅

@notnownikki
Copy link
Member Author

@tofumatt I'll add a comment :)

And everyone has permission to buy a giant foam bat with "embed e2e" tests written on to bop me on the head with. They are coming.

@tofumatt
Copy link
Member

tofumatt commented Oct 9, 2018

OMG now I want that bat for everyone! 😆

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.

3 participants