{"action":"created","issue":{"url":"https://api.github.com/repos/Automattic/amp-wp/issues/937","repository_url":"https://api.github.com/repos/Automattic/amp-wp","labels_url":"https://api.github.com/repos/Automattic/amp-wp/issues/937/labels{/name}","comments_url":"https://api.github.com/repos/Automattic/amp-wp/issues/937/comments","events_url":"https://api.github.com/repos/Automattic/amp-wp/issues/937/events","html_url":"https://github.com/Automattic/amp-wp/pull/937","id":294927675,"number":937,"title":"Issue #864: Remove 'sizes' attribute for <amp-iframe> and <amp-video>.","user":{"login":"kienstra","id":4063887,"avatar_url":"https://avatars2.githubusercontent.com/u/4063887?v=4","gravatar_id":"","url":"https://api.github.com/users/kienstra","html_url":"https://github.com/kienstra","followers_url":"https://api.github.com/users/kienstra/followers","following_url":"https://api.github.com/users/kienstra/following{/other_user}","gists_url":"https://api.github.com/users/kienstra/gists{/gist_id}","starred_url":"https://api.github.com/users/kienstra/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kienstra/subscriptions","organizations_url":"https://api.github.com/users/kienstra/orgs","repos_url":"https://api.github.com/users/kienstra/repos","events_url":"https://api.github.com/users/kienstra/events{/privacy}","received_events_url":"https://api.github.com/users/kienstra/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"westonruter","id":134745,"avatar_url":"https://avatars2.githubusercontent.com/u/134745?v=4","gravatar_id":"","url":"https://api.github.com/users/westonruter","html_url":"https://github.com/westonruter","followers_url":"https://api.github.com/users/westonruter/followers","following_url":"https://api.github.com/users/westonruter/following{/other_user}","gists_url":"https://api.github.com/users/westonruter/gists{/gist_id}","starred_url":"https://api.github.com/users/westonruter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/westonruter/subscriptions","organizations_url":"https://api.github.com/users/westonruter/orgs","repos_url":"https://api.github.com/users/westonruter/repos","events_url":"https://api.github.com/users/westonruter/events{/privacy}","received_events_url":"https://api.github.com/users/westonruter/received_events","type":"User","site_admin":false},"assignees":[{"login":"westonruter","id":134745,"avatar_url":"https://avatars2.githubusercontent.com/u/134745?v=4","gravatar_id":"","url":"https://api.github.com/users/westonruter","html_url":"https://github.com/westonruter","followers_url":"https://api.github.com/users/westonruter/followers","following_url":"https://api.github.com/users/westonruter/following{/other_user}","gists_url":"https://api.github.com/users/westonruter/gists{/gist_id}","starred_url":"https://api.github.com/users/westonruter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/westonruter/subscriptions","organizations_url":"https://api.github.com/users/westonruter/orgs","repos_url":"https://api.github.com/users/westonruter/repos","events_url":"https://api.github.com/users/westonruter/events{/privacy}","received_events_url":"https://api.github.com/users/westonruter/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/Automattic/amp-wp/milestones/6","html_url":"https://github.com/Automattic/amp-wp/milestone/6","labels_url":"https://api.github.com/repos/Automattic/amp-wp/milestones/6/labels","id":2895463,"number":6,"title":"v0.7","description":"Coming in 2018 after 0.6","creator":{"login":"westonruter","id":134745,"avatar_url":"https://avatars2.githubusercontent.com/u/134745?v=4","gravatar_id":"","url":"https://api.github.com/users/westonruter","html_url":"https://github.com/westonruter","followers_url":"https://api.github.com/users/westonruter/followers","following_url":"https://api.github.com/users/westonruter/following{/other_user}","gists_url":"https://api.github.com/users/westonruter/gists{/gist_id}","starred_url":"https://api.github.com/users/westonruter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/westonruter/subscriptions","organizations_url":"https://api.github.com/users/westonruter/orgs","repos_url":"https://api.github.com/users/westonruter/repos","events_url":"https://api.github.com/users/westonruter/events{/privacy}","received_events_url":"https://api.github.com/users/westonruter/received_events","type":"User","site_admin":false},"open_issues":16,"closed_issues":82,"state":"open","created_at":"2017-11-07T21:45:17Z","updated_at":"2018-02-27T22:00:40Z","due_on":"2018-02-09T08:00:00Z","closed_at":null},"comments":16,"created_at":"2018-02-06T21:58:28Z","updated_at":"2018-03-05T13:15:51Z","closed_at":null,"author_association":"COLLABORATOR","pull_request":{"url":"https://api.github.com/repos/Automattic/amp-wp/pulls/937","html_url":"https://github.com/Automattic/amp-wp/pull/937","diff_url":"https://github.com/Automattic/amp-wp/pull/937.diff","patch_url":"https://github.com/Automattic/amp-wp/pull/937.patch"},"body":"**Request For Code Review**\r\n\r\nHi @westonruter,\r\nCould you please review this pull request, which removes the `sizes` attribute for `<amp-iframe>` and `<amp-video>`?\r\n\r\nBefore, there was a workaround to ensure these didn't overflow. I had added `style=\"max-width:100%\"`\r\nThis removes that workaround, and instead sets the layout to 'responsive.'\r\n\r\nWe don't need to remove `sizes` from `<amp-img>`, right? The discussion in [PR #921](https://github.com/Automattic/amp-wp/pull/921) only briefly mentioned `<amp-img>`, though the referenced [amphtml issue # 11575](https://github.com/ampproject/amphtml/issues/11575) discusses `<amp-img>`.\r\n\r\n**Update**\r\nSorry, Weston, I might have missed the real need of removing `sizes`. It sounds like the main need was removing them from images. If there's still a need to remove them from images, I'd be happy to do that.\r\n"},"comment":{"url":"https://api.github.com/repos/Automattic/amp-wp/issues/comments/370415351","html_url":"https://github.com/Automattic/amp-wp/pull/937#issuecomment-370415351","issue_url":"https://api.github.com/repos/Automattic/amp-wp/issues/937","id":370415351,"user":{"login":"delawski","id":478735,"avatar_url":"https://avatars1.githubusercontent.com/u/478735?v=4","gravatar_id":"","url":"https://api.github.com/users/delawski","html_url":"https://github.com/delawski","followers_url":"https://api.github.com/users/delawski/followers","following_url":"https://api.github.com/users/delawski/following{/other_user}","gists_url":"https://api.github.com/users/delawski/gists{/gist_id}","starred_url":"https://api.github.com/users/delawski/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/delawski/subscriptions","organizations_url":"https://api.github.com/users/delawski/orgs","repos_url":"https://api.github.com/users/delawski/repos","events_url":"https://api.github.com/users/delawski/events{/privacy}","received_events_url":"https://api.github.com/users/delawski/received_events","type":"User","site_admin":false},"created_at":"2018-03-05T13:15:51Z","updated_at":"2018-03-05T13:15:51Z","author_association":"NONE","body":"@westonruter @kienstra So it turns out that for a larger logo file the aspect ratio of the image was not preserved. I've updated the CSS so that this should not be the case for the logo.\r\n\r\nHowever, the bigger issue I see is that all the images that are used in the post content don't have any `layout` value set. I think that setting `layout=\"responsive\"` would be a good default value to use. Otherwise the `layout=\"fixed\"` value is set by AMP which is not the way the images are sized on the non-AMP pages.\r\n\r\nWhen it comes to the featured article image, the aspect ratio / crop you can see on https://dev-ampconfdemo.pantheonsite.io/ is incorrect according to the mock-ups in Figma. In the previous implementation it was one of the limitations we didn't really have time to overcome. All images were using the same aspect ratio. Now, as we use `object-fit` and `layout=\"fill\"` for the posts' featured images, we can use the correct crops."}}
{"actor":{"display_login":"delawski"}}