Skip to content

🖍 Allow @page CSS at-rule#17481

Merged
Gregable merged 2 commits intoampproject:masterfrom
xwp:add/page-css-at-rule
Oct 31, 2018
Merged

🖍 Allow @page CSS at-rule#17481
Gregable merged 2 commits intoampproject:masterfrom
xwp:add/page-css-at-rule

Conversation

@westonruter
Copy link
Copy Markdown
Member

When testing the WordPress AMP plugin with the Genesis Sample Theme, the validator was raising an error regrading an @page at-rule appearing in the CSS:

@media print {
	/* ... */
	@page {
		margin: 2cm 0.5cm;
	}
	/* ... */
}

See MDN docs on @page rule: https://developer.mozilla.org/en-US/docs/Web/CSS/@page

There doesn't seem to be any reason for why @page should be excluded in AMP, other than it was perhaps not included since it is not a commonly-used rule. So this PR allows the rule in the AMP and AMP4EMAIL formats; I didn't include AMP4ADS as it didn't seem to make sense, but maybe it does.

@westonruter westonruter force-pushed the add/page-css-at-rule branch from e407e2a to fad0ac8 Compare August 14, 2018 01:17
@aghassemi aghassemi requested review from Gregable and aghassemi and removed request for aghassemi August 14, 2018 21:16
@aghassemi
Copy link
Copy Markdown
Contributor

Thanks @westonruter
/to @Gregable for review.

@Gregable
Copy link
Copy Markdown
Member

@westonruter Thanks for the contribution. We looked over the at-rules we found in a large sample of documents when making the initial list, so I suspect that this is just an oversight. I'd like @honeybadgerdontcare , who implemented the list to take a look but I see no problem.

@Gregable
Copy link
Copy Markdown
Member

I was confused. This was something else. Yes, the at-rule list was defined here:
https://www.ampproject.org/docs/fundamentals/spec#@-rules

I'm not sure who would be best to make this call. Probably @cramforce

Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

LGTM

Would be great if you could also update https://www.ampproject.org/docs/fundamentals/spec#@-rules

@westonruter
Copy link
Copy Markdown
Member Author

Would be great if you could also update https://www.ampproject.org/docs/fundamentals/spec#@-rules

Done in e472360.

@ericlindley-g
Copy link
Copy Markdown
Contributor

Checking in — @cramforce , @Gregable , does anything else need to be done here in order to merge this PR? Do you have a sense of the timeline until this would become available?

/cc @amedina

@westonruter
Copy link
Copy Markdown
Member Author

Anything else needed here?

@Gregable
Copy link
Copy Markdown
Member

Looks good to me.

@Gregable Gregable merged commit e041b39 into ampproject:master Oct 31, 2018
@westonruter westonruter deleted the add/page-css-at-rule branch October 31, 2018 17:19
jpettitt added a commit that referenced this pull request Oct 31, 2018
twifkak added a commit to twifkak/amphtml that referenced this pull request Nov 2, 2018
@twifkak twifkak mentioned this pull request Nov 2, 2018
twifkak added a commit that referenced this pull request Nov 3, 2018
* cl/219531121 Revision bump for #17481

* cl/219850294 Allow `http` scheme for links in email spec

* cl/219866890 Revision bump for #19043

* cl/219867113 Revision bump for #18981

* cl/219877087 Make ESlint happy.

* cl/219882876 Fix lint for #19096
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
* Allow page CSS at-rule

* Update docs to include `@page` among allowed CSS at-rules
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
* cl/219531121 Revision bump for ampproject#17481

* cl/219850294 Allow `http` scheme for links in email spec

* cl/219866890 Revision bump for ampproject#19043

* cl/219867113 Revision bump for ampproject#18981

* cl/219877087 Make ESlint happy.

* cl/219882876 Fix lint for ampproject#19096
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants