Skip to content

Conversation

@philipjohn
Copy link
Contributor

Changes proposed in this Pull Request:

The CSS for the share icons triggers a "illegal_css_at_rule" AMP validation error because of the use of '-moz-document'. Given AMP uses the amp-social-share component, this CSS isn't needed at all. This change tells Jetpack not to enqueue the CSS at all when an AMP page is being viewed.

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

This builds upon existing AMP compatibility fixes.

Testing instructions:

  1. Enable the AMP plugin in Transitional mode
  2. Enable social sharing icons in Jetpack
  3. View a post, and observe the illegal_css_at_rule validation error
  4. Apply the PR, and refresh the post to see the validation passes

Proposed changelog entry for your changes:

  • AMP compatibility fix for sharing button CSS

The CSS for the share icons triggers a "illegal_css_at_rule" AMP validation error because of the use of '-moz-document'. Given AMP uses the `amp-social-share` component, this CSS isn't needed at all. This change tells Jetpack not to enqueue the CSS at all when an AMP page is being viewed.
@philipjohn philipjohn requested a review from a team July 4, 2019 16:48
@matticbot
Copy link
Contributor

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

@philipjohn philipjohn added [Type] Bug When a feature is broken and / or not performing as intended AMP [Feature] Sharing Post sharing, sharing buttons labels Jul 4, 2019
@jetpackbot
Copy link
Collaborator

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against a61a5cf

@westonruter
Copy link
Contributor

This brings up something else: shouldn't the social sharing buttons in AMP get settled the same way as they do on the non-AMP page? The amp-social-share elements are styleable.

@philipjohn philipjohn added the [Status] Needs Review This PR is ready for review. label Jul 4, 2019
@jeherve jeherve added this to the 7.6 milestone Jul 25, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works in my tests. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jul 25, 2019
@jeherve jeherve merged commit fda620a into master Jul 29, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 29, 2019
@jeherve jeherve deleted the fix/social-share-css-amp-compat branch July 29, 2019 09:13
jeherve added a commit that referenced this pull request Jul 29, 2019
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AMP [Feature] Sharing Post sharing, sharing buttons 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.

6 participants