Skip to content

Skip replacing the embed markup on AMP pages - #54

Merged
kevinweber merged 2 commits into
kevinweber:masterfrom
mjaschen:feature/disable-on-amp-pages
Mar 27, 2021
Merged

Skip replacing the embed markup on AMP pages#54
kevinweber merged 2 commits into
kevinweber:masterfrom
mjaschen:feature/disable-on-amp-pages

Conversation

@mjaschen

Copy link
Copy Markdown
Contributor

When using the “AMP for WordPress” plugin, the <noscript> message is shown („Video can't be loaded because JavaScript is disabled“).

This pull request checks if the current request is for an AMP page and skips replacing the embed markup in this case.

}

$isAmp = function_exists('amp_is_request') && amp_is_request();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@mjaschen Thank you for the contribution, very much appreciated!

This is maybe a bit nit-picky but we can make the code a little bit more readable by using an early return instead of having !$isAmp && (! is_feed()) in two conditions:

$isAmp = function_exists('amp_is_request') && amp_is_request();
if ($isAmp || is_feed()) return $return;

What do you think?

Either way, I'll get to merging and publishing this change on the upcoming weekend.

Thanks again for contributing!

@mjaschen

mjaschen commented Mar 23, 2021

Copy link
Copy Markdown
Contributor Author

Hi @kevinweber

you're absolutely right. I've updated the checks and added early returns.

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.

2 participants