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

AMP: allow the display of the admin bar in AMP views. #15372

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Apr 8, 2020

Fixes #15353

Changes proposed in this Pull Request:

  • The admin bar can be used in AMP views since AMP 1.3.1, thanks to AMP Dev mode. We consequently do not need to force-deactivate it.
  • Since Likes aren't yet compatible with AMP yet (see Port Likes and Comment Likes to work with AMP #9555), let's remove the Likes iFrame from the admin bar.

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

Testing instructions:

  • On a brand new site running Twenty Twenty, this branch as well as the most recent of the AMP plugin, go to AMP > General and switch to the "Standard" option.
  • Go to your site's frontend; you should still see the admin bar. You should also see no valdation errors linked to the Likes or Comment Likes module.

image

  • You should also not see any empty space in the admin bar, where the Likes button previously was.

  • Alternatively, you can also try to activate and configure an ad on your site using the advanced-ads plugin, and make sure no warnings are generated in your error when viewing your site's frontend.

Proposed changelog entry for your changes:

  • General: avoid conflicts with other plugins interacting with the AMP plugin and the admin bar.

Fixes #15353

The admin bar can be used in AMP views since AMP 1.3.1, thanks to AMP Dev mode.
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from Crew. Label will be renamed soon. AMP labels Apr 8, 2020
@jeherve jeherve added this to the 8.4.2 milestone Apr 8, 2020
@jeherve jeherve requested a review from a team as a code owner April 8, 2020 15:58
@jeherve jeherve self-assigned this Apr 8, 2020
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Apr 8, 2020

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: May 5, 2020.
Scheduled code freeze: April 28, 2020

Generated by 🚫 dangerJS against 45d7b03

@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D41598-code has been updated.

@sergeymitr sergeymitr self-requested a review April 8, 2020 17:32
@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D41598-code has been updated.

Copy link
Contributor

@sergeymitr sergeymitr left a comment

Choose a reason for hiding this comment

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

Works great now, nicely done!

Copy link
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

This looks good to me! I followed the provided testing instructions, and the admin bar looks good. No validation errors were generated.

I also activated the advanced-ads plugin and added the example ads to my test site. I’m not sure if that was a sufficient test, but no errors were generated on my test site.

@kbrown9 kbrown9 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 14, 2020
@jeherve jeherve merged commit 0a437ce into master Apr 14, 2020
@jeherve jeherve deleted the fix/amp-admin-bar branch April 14, 2020 12:15
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 14, 2020
jeherve added a commit that referenced this pull request Apr 14, 2020
…15372)

* AMP: allow the display of the admin bar in AMP views.

Fixes #15353

The admin bar can be used in AMP views since AMP 1.3.1, thanks to AMP Dev mode.

* Likes: do not load Likes in admin bar on AMP views
@jeherve
Copy link
Member Author

jeherve commented Apr 14, 2020

Cherry-picked to branch-8.4 in 95d4b87

@jeherve
Copy link
Member Author

jeherve commented Apr 14, 2020

r205796-wpcom

jeherve added a commit that referenced this pull request Apr 14, 2020
* 8.4.2 changelog: add #15365

* [not verified] Changelog: add #15372

* Changelog: add #15368

* Changelog: add #15395

* Changelog: add #15396

* Changelog: add #15406
jeherve added a commit that referenced this pull request Apr 14, 2020
* 8.4.2 changelog: add #15365

* [not verified] Changelog: add #15372

* Changelog: add #15368

* Changelog: add #15395

* Changelog: add #15396

* Changelog: add #15406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AMP: Jetpack now always hides the admin bar
6 participants