-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
SSR: Add support for media, sizes and heights attribute #763
Conversation
6546d5f
to
7065ef1
Compare
@westonruter @schlessera PTAL - I haven't yet implemented the CSS size check. |
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.
I reviewed what I could, but @schlessera is better suited to give more in-depth feedback.
|
||
const parseSizes = require('../parseSizes'); | ||
const {appendChild, createElement, insertText, hasAttribute} = require('../NodeUtils'); | ||
const ID_PREFIX = 'i-amp-'; |
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.
The prefix i-amphtml-
is disallowed, but i-amp-
is OK. It seems somewhat strange why i-amp-
wasn't chosen rather for the internal prefix since it has fewer bytes.
appendChild(head, customStyles); | ||
insertText(customStyles, ''); | ||
} | ||
customStyles.children[0].data += styles; |
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.
As noted in the description, we'll need to make sure this doesn't go above 75000 bytes.
7fd8734
to
7065ef1
Compare
|
||
transform(node, id) { | ||
// normalize whitespace | ||
let mediaString = node.attribs.media.replace(/\s+/g, ' '); |
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.
Seems like this is already done within parseSizes()
?
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.
parseSizes is unrelated to the media attribute or am I missing something?
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.
Oh, nvm, I was thinking of the media conditions that are part of the sizes
attribute.
* | ||
* @param {Node} node | ||
*/ | ||
getOrCreateId(node) { |
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.
This method doesn't check whether it is maybe creating duplicate IDs. This will break if it runs on markup that is already partially or fully optimized, or which happens to use the same prefix.
The PHP version will detect pre-existing IDs and iterate over them until it finds an ID that is not yet taken: https://github.com/ampproject/amp-wp/blob/63e99a63ca0624d5cff8f6e8d8d1ac85b89e337b/lib/common/src/Dom/Document.php#L1509-L1512
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.
True. This is not straightforward to implement in our case unfortunately.
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.
I'll ignore this case for now. Re-running the transformer would simply re-use the existing id. Pre-existing Ids should not be avoided by our prefix.
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.
Done
<noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> | ||
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both; | ||
-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes-amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style> | ||
<noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> |
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.
Why the change in order here?
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.
no idea!
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.
reverted
|
||
|
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.
Removal of the boilerplate shouldn't leave empty lines behind.
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.
that's going to be removed by a different transformer (not used in the test)
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.
This will be fixed by a different transformer
packages/optimizer/lib/parseSizes.js
Outdated
* See https://html.spec.whatwg.org/multipage/images.html#parsing-a-sizes-attribute | ||
* | ||
* @param {String} string | ||
* @returns {!Sizes} |
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.
Why the "non-nullable" modifier !
here specifically? Wouldn't that be the default without the modifier anyways? It seems like that distinction isn't use anywhere else.
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.
It's a closure compiler flag used by the AMP runtime. Removed it as we don't need it.
I've addressed the comments. In regards to checking for the CSS limit. I'm currently lending towards not including this check:
|
@westonruter @schlessera PTAL |
Doesn't this run the risk of the Optimizer generating invalid AMP from valid AMP input? If the CSS is close to the limit and then optimization causes it to go over, that seems not ideal? |
6636719
to
50b0bdc
Compare
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.
Optimizer changes CSS for the better and for the worse. This means you should run validation afterwards. I'm OK with failing validation if CSS impacts perf.
|
||
|
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.
This will be fixed by a different transformer
* | ||
* @param {Node} node | ||
*/ | ||
getOrCreateId(node) { |
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.
Done
Merging as there's no further feedback that needs to be addressed. |
@sebastianbenz There seems to be an issue with the CSS in here that was raised twice but always resolved without a comment, so I assume that GitHub resolved it because of an unrelated change: #763 (comment) Or am I missing something maybe? |
Hah! Not sure how this got lost. Fixed in #811 |
Thanks for picking up on this! |
This will make it possible to remove the boilerplate even if the
media
,sizes
orheights
attributes are used. These attributes will be transformed into CSS media queries instead.