Open pietrop opened 6 years ago
@pietrop this definitely looks like a bug/missing feature in the latest version of this module, as there is no way to stretch image to fill the entire height while cropping on the sides. Our previous version of this module had somewhat different json structure that allowed this, but it also had other issues.
I think what's missing is something similar to resizeMode=cover
from react native https://facebook.github.io/react-native/docs/image.html#style. Otherwise, not sure this can be expressed through flexbox only.
On validator tool, it's pretty experimental, might not always be accurate and errors aren't great. I've been looking into another json validator tool that can handle union types better, but haven't found anything yet.
Oh, and the reason we wrap everything into a div is to abstract layout from AMP default styles and rely fully on flexbox. This should allow us to make it less dependent on AMP default container styles/layout, so it's portable to react native / video etc.
Thanks @iefserge, Any thoughts on what would be the best way to address the image distortion in this module? Happy to have a go at it with some guidance.
@pietrop I think we add resizeMode
property to images, which can use AMP layout mode and/or extra div/css to scale it correctly.
https://www.ampproject.org/docs/design/responsive/control_layout
https://ampbyexample.com/advanced/how_to_support_images_with_unknown_dimensions/
@iefserge how would that be different from the current layout
option?
@pietrop We copied it from AMP initially, but actually ignore it at the moment. I'll remove it from the example. I think flexbox should cover most layout cases, except for the image sizing mode.
When rendering stories into videos, we use https://github.com/facebook/yoga to compute layout, so it would be hard to implement non-flex layout options.
If anyone faces the same issues, I added a align-self: center;
to the amp-img container.
Using
story-json
to create AMP stories for BBC News Labs, we found that theamp-img
tag shows the image as distorted. The image width is squashed in the various mobile screen sizes, breaking the aspect ratio of the image. And was wondering if it something we are doing wrong in the use of the json file schema as we didn't see this issue in Mic News published AMP stories.Here's a test example:
But if we "manually" move it, using the browser inspector, outside of the two divs that you can see in the screenshots above seem to be containing it, it is no longer distorted. As you can see below.
This is the
story-json
we have been working with, I left only one page in it for simplicity.And these are some of the errors from the validator for that page, they seem a bit cryptic, so not sure what to make of it.
Looking at this story from Mic News
arctic-sea-ice-is-disappearing-heres-why-you-should-care
The background images get cropped on the right size when changing device size. eg from iphone 5 to iphone X. (look at the nose of the polar bear).
iphone 5
Iphone X
This is what the AMP HTML for that card/page looks like in the inspector
The first
amp-img
for the background image in the in the amp html page is a sibling ofamp-story-grid-layer
without any nested divs.If possible, it could be useful for us to see what that page looks like in the
story-json
format for troubleshooting?Thanks for your time