-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line between declarations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@cogell All looks good. I have two minor comments. And, yes, please do add a section to the |
on it... |
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.) |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"amp css" -> "AMP CSS"
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