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

Eliminate amp-wp-enforced-sizes style from theme support stylesheet #1153

Merged
merged 1 commit into from
May 17, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 17, 2018

This style rule causes problems on AMP Native sites (where amp theme support is present). An image element may have margins and width associated with it which the amp-wp-enforced-sizes style rule clobbers unexpectedly.

For example, in non-AMP given:

image

With CSS properties:

.logo {
    display: block;
    margin: 2rem 0 0;
    width: 300px;
    max-width: 100%;
    margin-bottom: 2rem;
}

This gets rendered in AMP as:

image

Since the following override:

.amp-wp-enforced-sizes {
    max-width: 100%;
    margin: 0 auto;
}

I don't think this is relevant in an amp theme support context. I believe it was only relevant to AMP legacy post templates and can be removed now.

@westonruter westonruter added this to the v1.0 milestone May 17, 2018
@westonruter westonruter requested a review from kienstra May 17, 2018 08:25
Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @westonruter,
Thanks, good idea to remove that style rule when theme support is present. I think you're right that it doesn't make sense outside of legacy templating.

@westonruter westonruter merged commit ab4b7f5 into develop May 17, 2018
@westonruter westonruter deleted the update/amp-default-stylesheet branch May 17, 2018 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants