Skip to content
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

Pinterest responsive layout not working #7597

Closed
tjwelde opened this issue Feb 16, 2017 · 6 comments
Closed

Pinterest responsive layout not working #7597

tjwelde opened this issue Feb 16, 2017 · 6 comments
Assignees
Labels
Milestone

Comments

@tjwelde
Copy link

tjwelde commented Feb 16, 2017

If you try to embed a pinterest widget with the layout option "responsive". The Pinterest widget is broken.

How do we reproduce the issue?

  1. Add pinterest widget as described on https://ampbyexample.com/components/amp-pinterest/
  2. Add layout="responsive" to the amp-pinterest tag
  3. Open in any browser

Here you can see the issue:
http://jsbin.com/nekomecida/edit?html,output

It should look like this:
http://jsbin.com/yesavaqaqu/edit?html,output

Which AMP version is affected?

1486670309274

@sebastianbenz
Copy link
Contributor

//cc @paul-matthews

@aghassemi
Copy link
Contributor

I would have expected a responsive amp-pinterest to look like this: http://jsbin.com/gomafapago/1/edit?html,output

@christianvuerings, @kentbrew related to #7320 I am curious about

.-amp-pinterest-embed-pin-responsive .-amp-pinterest-embed-pin-image {
  max-width: 100%;
}

instead of

.-amp-pinterest-embed-pin-responsive .-amp-pinterest-embed-pin-image {
  width: 100%;
}

@christianvuerings
Copy link
Contributor

@goldquest Seems like the fix in #7320 has been merged but hasn't been released yet. Currently it's part of the following pre-release: https://github.com/ampproject/amphtml/releases/tag/1486769961849

@aghassemi The reason we've used max-width is so we don't show pixelated images to our users.

@aghassemi
Copy link
Contributor

@christianvuerings Could we integrate srcset with the amp-pinterest? I assume pinterest has cached multiple resolutions for these images. To be consistent with amp-image, layout=responsive should fill the width with the image. srcset should mitigate the pixelation problem allowing width: 100%; to be applied.

@adelinamart adelinamart added this to the Backlog Bugs milestone Feb 23, 2017
@christianvuerings
Copy link
Contributor

@aghassemi that should probably work up until a certain resolution.
Also tagging @kentbrew so he's aware of this proposal.

@aghassemi
Copy link
Contributor

Closing since #7320 is in production.

@ericlindley-g ericlindley-g added this to Needs Triage in UI Apr 14, 2017
@ericlindley-g ericlindley-g moved this from Needs Triage to Done in UI Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants