Amphtml: amp-pinterest ignores `data-height="tall"`

Created on 19 Jul 2017  路  6Comments  路  Source: ampproject/amphtml

What's the issue?

data-height="tall" is not changing the how the Pinterest button renders.

How do we reproduce the issue?

In the AMP playground, if you change the Pinterest button code to

<amp-pinterest height="28" width="56"
      data-do="buttonPin"
      data-url="https://ampbyexample.com/"
      data-media="https://ampbyexample.com/img/amp.png"
      data-description="amp-pinterest in action"
      data-height="tall" >
    </amp-pinterest>

per the documentation: https://www.ampproject.org/docs/reference/components/amp-pinterest
You see this
screenshot 2017-07-19 14 09 46

Similarly, if you try to create a large round button:

 <amp-pinterest height="32" width="32"
      data-do="buttonPin"
      data-url="https://ampbyexample.com/"
      data-media="https://ampbyexample.com/img/amp.png"
      data-description="amp-pinterest in action"
      data-height="tall" data-round="true" >
    </amp-pinterest>

You get this:
screenshot 2017-07-19 14 13 47

What browsers are affected?

Testing on latest Chrome, MacOS 10.12.6

Which AMP version is affected?

AMP 1499807487098

GFI Candidate When Possible

All 6 comments

/cc @levidurfee

hi @chenryhen !

Thank you for submitting this issue. I'll be looking into it this weekend. I'll let you know what I find or if I have any questions :)

@chenryhen , would you mind trying something for me?

If you change your data-height attribute to data-tall, then the value _should be_ true, but I think it'll work with anything in there.

I was looking at the docs for this component and it does say data-height. However I don't see anything in the code that uses data-height.

When you get a chance, please let me know if that works for you.

I still think there may be an issue with margins for buttons with a pin count.

Edit: Appended last comment

@levidurfee that works! Thanks

That is great news, @chenryhen ! Thank you for taking the time to try that.

I'll check to see I should create a pull request for the other things I noticed while looking into this.

/cc @aghassemi

@levidurfee Thanks for looking into this.

Was this page helpful?
0 / 5 - 0 ratings