Add amp-border-box class for use on html #1508
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 1a694315b5e237d9769cb5ceff2b3c277c048224 (Please note that this is a fully automated comment.) |
| html.amp-border-box *, | ||
| html.amp-border-box *:before, | ||
| html.amp-border-box *:after { | ||
| box-sizing: inherit; |
There was a problem hiding this comment.
Hmm. By default though box-sizing is not inherited. This could cause some unexpected behavior where someone would override box-sizing just for one element and it'd automatically apply to everything within.
What about if we just explicitly state border-box instead of inherit?
There was a problem hiding this comment.
Great point. My thinking was that html components that are built around a different box-sizing model would only need to set their box-sizing on the parent which would inherit down. Now I'm thinking that that case in an AMP page would be rare and in that edge case the author could assign box-sizing on each element on that component.
Will update.
cogell
force-pushed
the
Add-amp-border-box-class
branch
from
1a69431
to
6ae0bfc
Compare
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 6ae0bfc3488d64002997620d300f7461e37bd10c (Please note that this is a fully automated comment.) |
|
@cogell sorry, not sure, is this already updated and ready to be reviewed? |
|
@dvoytenko sorry forget to ping. Yes it is updated. looks like the CI tests failed for a connection issue. |
|
Should I add this new css class in the |
| html.amp-border-box { | ||
| box-sizing: border-box; | ||
| } | ||
| html.amp-border-box *, |
There was a problem hiding this comment.
nit: new line between declarations
|
@cogell All looks good. I have two minor comments. And, yes, please do add a section to the |
|
on it... |
cogell
force-pushed
the
Add-amp-border-box-class
branch
2 times, most recently
from
d19e56c
to
27b6b9d
Compare
|
@dvoytenko updated! |
|
Is there a reason to restrict this to |
|
hm! sure it seems like this would be fine too: |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 27b6b9d5b29038d70f192dba65c0eccff40d387d (Please note that this is a fully automated comment.) |
cogell
force-pushed
the
Add-amp-border-box-class
branch
from
27b6b9d
to
18bcd2d
Compare
|
@jridgewell updated |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: d19e56c46fecb5a50c17f19a50809b672ec2b2bd (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 18bcd2d (Please note that this is a fully automated comment.) |
| @@ -241,6 +241,11 @@ if the browser doesn't support HTML5 audio, for example: | |||
| <source type="audio/ogg" src="foo.ogg"> | |||
| </amp-audio> | |||
| ``` | |||
| # Add Border Box Sizing | |||
|
|
|||
| Included in the base amp css is a class of `amp-border-box` that will set `box-sizing: border-box` on all elements | |||
|
Great! LGTM with one minor comment. |
Add amp-border-box class for use on html
|
Merged. Thanks a lot! |
|
Please guide me with this PR: It adds box-sizing to all elements but it can be easily overriden. |
Quick pass at: #1497