🐛Modify amp-pinterest.css from displaying "Pin It" to "Save" as a button #20706

caroqliu · 2019-02-06T19:15:24Z

Resolves #18648

Modifies existing CSS selectors and adds ".-amp-pinterest-save-button" and ".-amp-pinterest-save-button-tall" selectors.
Updates element dimensions in example html file "pinterest.amp.html".
Does not support language codes other than 'ja' and 'en', which Pinterest does.

… Save buttons.

…isible again.

cvializ · 2019-02-06T19:27:19Z

examples/pinterest.amp.html


  <h4>Small Gray English with No Count</h4>

-    <amp-pinterest height=20 width=40
+    <amp-pinterest height=20 width=50


If it needs to be this much higher to render correctly I think it might be a breaking change, since the already-published amp documents will have the old dimensions in the markup.

Ok, I added some commits that comply with the existing dimensions. Let me know how things look now.

Nice, a side by side comparison looks really good!

Before After

Where did you get the gray value for the gray buttons?

I took it from the colors of the Follow Button since there is not brand guidelines on a standard gray value. Optionally I could also grab from the bubble text color (#555)?

Yeah we kind of have to choose something ourselves. The current gray seems a little dark to me. I think the bubble text color could look better but maybe @andrewwatterson has an opinion on what gray would look best here

Nice, love the side-by-side screenshot.

On the widget, it still says "Pin", is that change not part of this PR? (if plan is to change that in a different PR, let's make sure they both go out in the same Canary cut.)

Regarding Gray, follow button I see on https://developers.pinterest.com/docs/widgets/profile-widget/ is a bit different. It has box-shadow: inset 0 0 1px #888; background-color: #efefef; I also see Pinterest no longer supporting data-pin-color and it is always red. It don't mind us doing the same and breaking color variable all together. At very minimum we should change the default to be red (right now it is gray)

I wasn't planning on modifying the PinWidget in this PR but in retrospect it will be nice for these changes to be done as a unit. I'll send the new change shortly.

How about gray with background #efefef and color #555, to match the bubble style? It's softer/easier to look at, though a case could be made it is more muted than we'd want it to be.

…ions.

cvializ

This is a really great PR for your first AMP issue! The issue turned out to be more complex than we usually aim to assign for first issues, but it's going well.

cvializ · 2019-02-07T15:10:24Z

extensions/amp-pinterest/0.1/amp-pinterest.css

+  color: #e60023;
+  background-color: #fff;
+  box-sizing: border-box;
+  box-shadow: inset 0 0 1px #e60023;


Is the inset shadow spec'd somewhere by pinterest?

Not explicitly spec'd but followed the style from inspecting on the Widget Builder: https://developers.pinterest.com/tools/widget-builder/?type=follow

cvializ · 2019-02-07T15:21:36Z

There are a couple of errors on Travis: https://travis-ci.org/ampproject/amphtml/builds/489766406

It looks like Pinterest saved their SVG data-uri encoded as base64, but AMP has a presubmit check to force all SVG data-uris to be encoded as utf-8.

And you'll need to add the renamed save-button.js to the whitelist of components that are allowed to use the old internal classname format -amp-

amphtml/build-system/tasks/presubmit-checks.js

Lines 50 to 61 in 63fda3c

    
             message: 'Switch to new internal class form', 
        
             whitelist: [ 
        
               'build-system/amp4test.js', 
        
               'build-system/app-index/boilerplate.js', 
        
               'build-system/tasks/extension-generator/index.js', 
        
               'css/amp.css', 
        
               'extensions/amp-pinterest/0.1/amp-pinterest.css', 
        
               'extensions/amp-pinterest/0.1/follow-button.js', 
        
               'extensions/amp-pinterest/0.1/pin-widget.js', 
        
               'extensions/amp-pinterest/0.1/pinit-button.js', 
        
             ], 
        
           },

caroqliu · 2019-02-07T15:52:52Z

Forgot to ask-- is there a guideline about alphabetizing (or otherwise ordering) CSS attributes? I did this with some of the selectors I modified but others that I didn't modify didn't always show evidence of this being the case.

cvializ · 2019-02-07T16:14:45Z

Good question, no we don't enforce any ordering in our CSS right now.

danielrozenberg · 2019-02-07T16:16:43Z

Looks like bundle-size bot is struggling with this PR; let me know when you're ready to merge and I will approve this check for skip (it doesn't look like your PR changes any files that would affect v0.js anyways)

My approval will get lost if you push more commits, so to avoid multiple re-approvals I'll wait until you're ready

…ayscale.

…guidelines.

caroqliu · 2019-02-07T16:45:38Z

I modified the whitelist and made the change for the SVG data-uri from base64 to utf8.

Note: This change made the white border around the logo all but disappear. The SVG data I took from logo files provided on the Pinterest brand guidelines.

cvializ · 2019-02-07T18:22:08Z

I think it will need the white border, to make our button look like theirs. You can convert the base64 string to utf8 using the atob function in the browser console or Node.

const svg = atob('PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIGhlaWdodD0iMzBweCIgd2lkdGg9IjMwcHgiIHZpZXdCb3g9Ii0xIC0xIDMxIDMxIj48Zz48cGF0aCBkPSJNMjkuNDQ5LDE0LjY2MiBDMjkuNDQ5LDIyLjcyMiAyMi44NjgsMjkuMjU2IDE0Ljc1LDI5LjI1NiBDNi42MzIsMjkuMjU2IDAuMDUxLDIyLjcyMiAwLjA1MSwxNC42NjIgQzAuMDUxLDYuNjAxIDYuNjMyLDAuMDY3IDE0Ljc1LDAuMDY3IEMyMi44NjgsMC4wNjcgMjkuNDQ5LDYuNjAxIDI5LjQ0OSwxNC42NjIiIGZpbGw9IiNmZmYiIHN0cm9rZT0iI2ZmZiIgc3Ryb2tlLXdpZHRoPSIxIj48L3BhdGg+PHBhdGggZD0iTTE0LjczMywxLjY4NiBDNy41MTYsMS42ODYgMS42NjUsNy40OTUgMS42NjUsMTQuNjYyIEMxLjY2NSwyMC4xNTkgNS4xMDksMjQuODU0IDkuOTcsMjYuNzQ0IEM5Ljg1NiwyNS43MTggOS43NTMsMjQuMTQzIDEwLjAxNiwyMy4wMjIgQzEwLjI1MywyMi4wMSAxMS41NDgsMTYuNTcyIDExLjU0OCwxNi41NzIgQzExLjU0OCwxNi41NzIgMTEuMTU3LDE1Ljc5NSAxMS4xNTcsMTQuNjQ2IEMxMS4xNTcsMTIuODQyIDEyLjIxMSwxMS40OTUgMTMuNTIyLDExLjQ5NSBDMTQuNjM3LDExLjQ5NSAxNS4xNzUsMTIuMzI2IDE1LjE3NSwxMy4zMjMgQzE1LjE3NSwxNC40MzYgMTQuNDYyLDE2LjEgMTQuMDkzLDE3LjY0MyBDMTMuNzg1LDE4LjkzNSAxNC43NDUsMTkuOTg4IDE2LjAyOCwxOS45ODggQzE4LjM1MSwxOS45ODggMjAuMTM2LDE3LjU1NiAyMC4xMzYsMTQuMDQ2IEMyMC4xMzYsMTAuOTM5IDE3Ljg4OCw4Ljc2NyAxNC42NzgsOC43NjcgQzEwLjk1OSw4Ljc2NyA4Ljc3NywxMS41MzYgOC43NzcsMTQuMzk4IEM4Ljc3NywxNS41MTMgOS4yMSwxNi43MDkgOS43NDksMTcuMzU5IEM5Ljg1NiwxNy40ODggOS44NzIsMTcuNiA5Ljg0LDE3LjczMSBDOS43NDEsMTguMTQxIDkuNTIsMTkuMDIzIDkuNDc3LDE5LjIwMyBDOS40MiwxOS40NCA5LjI4OCwxOS40OTEgOS4wNCwxOS4zNzYgQzcuNDA4LDE4LjYyMiA2LjM4NywxNi4yNTIgNi4zODcsMTQuMzQ5IEM2LjM4NywxMC4yNTYgOS4zODMsNi40OTcgMTUuMDIyLDYuNDk3IEMxOS41NTUsNi40OTcgMjMuMDc4LDkuNzA1IDIzLjA3OCwxMy45OTEgQzIzLjA3OCwxOC40NjMgMjAuMjM5LDIyLjA2MiAxNi4yOTcsMjIuMDYyIEMxNC45NzMsMjIuMDYyIDEzLjcyOCwyMS4zNzkgMTMuMzAyLDIwLjU3MiBDMTMuMzAyLDIwLjU3MiAxMi42NDcsMjMuMDUgMTIuNDg4LDIzLjY1NyBDMTIuMTkzLDI0Ljc4NCAxMS4zOTYsMjYuMTk2IDEwLjg2MywyNy4wNTggQzEyLjA4NiwyNy40MzQgMTMuMzg2LDI3LjYzNyAxNC43MzMsMjcuNjM3IEMyMS45NSwyNy42MzcgMjcuODAxLDIxLjgyOCAyNy44MDEsMTQuNjYyIEMyNy44MDEsNy40OTUgMjEuOTUsMS42ODYgMTQuNzMzLDEuNjg2IiBmaWxsPSIjZTYwMDIzIj48L3BhdGg+PC9nPjwvc3ZnPg==')
console.log(svg);
<svg xmlns="http://www.w3.org/2000/svg" height="30px" width="30px" viewBox="-1 -1 31 31"><g><path d="M29.449,14.662 C29.449,22.722 22.868,29.256 14.75,29.256 C6.632,29.256 0.051,22.722 0.051,14.662 C0.051,6.601 6.632,0.067 14.75,0.067 C22.868,0.067 29.449,6.601 29.449,14.662" fill="#fff" stroke="#fff" stroke-width="1"></path><path d="M14.733,1.686 C7.516,1.686 1.665,7.495 1.665,14.662 C1.665,20.159 5.109,24.854 9.97,26.744 C9.856,25.718 9.753,24.143 10.016,23.022 C10.253,22.01 11.548,16.572 11.548,16.572 C11.548,16.572 11.157,15.795 11.157,14.646 C11.157,12.842 12.211,11.495 13.522,11.495 C14.637,11.495 15.175,12.326 15.175,13.323 C15.175,14.436 14.462,16.1 14.093,17.643 C13.785,18.935 14.745,19.988 16.028,19.988 C18.351,19.988 20.136,17.556 20.136,14.046 C20.136,10.939 17.888,8.767 14.678,8.767 C10.959,8.767 8.777,11.536 8.777,14.398 C8.777,15.513 9.21,16.709 9.749,17.359 C9.856,17.488 9.872,17.6 9.84,17.731 C9.741,18.141 9.52,19.023 9.477,19.203 C9.42,19.44 9.288,19.491 9.04,19.376 C7.408,18.622 6.387,16.252 6.387,14.349 C6.387,10.256 9.383,6.497 15.022,6.497 C19.555,6.497 23.078,9.705 23.078,13.991 C23.078,18.463 20.239,22.062 16.297,22.062 C14.973,22.062 13.728,21.379 13.302,20.572 C13.302,20.572 12.647,23.05 12.488,23.657 C12.193,24.784 11.396,26.196 10.863,27.058 C12.086,27.434 13.386,27.637 14.733,27.637 C21.95,27.637 27.801,21.828 27.801,14.662 C27.801,7.495 21.95,1.686 14.733,1.686" fill="#e60023"></path></g></svg>

… border around the Pinterest logo.

…f a 'Pin It' button.

extensions/amp-pinterest/0.1/save-button.js

cvializ

LGTM! After the last comment is addressed, we can merge this when the build is green 🙌

cvializ · 2019-02-08T17:22:59Z

Looks like the Percy review is an amp-date-picker flake, so it's ok to approve. Let me know when you're ready and I'll merge the PR

caroqliu · 2019-02-08T17:31:00Z

This is good to go on my end. Thanks for reviewing!

cvializ · 2019-02-08T17:32:15Z

🚢 🚢 🚢

…ton (ampproject#20706) * change references from 'Pin It' to 'Save' in amp-pinterest * Modify css for follow button and round logo classes * Modify Save Button class styles * Modify amp-pinterest classes to display colored and language-specific Save buttons. * Modify count-beside and count-below css classes to make Save button visible again. * Alphabetize css attributes. * Alphabetize css attributes. * Fix dimensions on save-button-tall. * Resolve misplacement of -count-above classes. * Add bottom arrow to -bubble-above. * Remove 'content' attribute. * Add side array to -bubble-beside. * Revert examples/pinterest.amp.html * Update -bubble-above and -bubble-beside classes to match fixed dimensions. * Whitelist renamed file save-button.js * Update SVG data-uri to be encoded as utf-8. * Refactor SVG data uri to remove inline style tags. * Refactor gray color classes to display the logo on Save Buttons in grayscale. * Revert changes to gray logo -- Realized this is not allowed in brand guidelines. * Replace SVG inline data with converted base64 data to restore a white border around the Pinterest logo. * Default button color to red instead of gray. * Update PinWidget.js (embedded pins) to show a 'Save' button instead of a 'Pin It' button. * Update styling for gray buttons to emulate bubble styling. * Revert changing to logging request. * Revert change to logging request.

caroqliu added 12 commits February 5, 2019 14:20

change references from 'Pin It' to 'Save' in amp-pinterest

a0ed537

Modify css for follow button and round logo classes

ab597a3

Modify Save Button class styles

b90bb63

Modify amp-pinterest classes to display colored and language-specific…

aac671e

… Save buttons.

Modify count-beside and count-below css classes to make Save button v…

bc461e1

…isible again.

Alphabetize css attributes.

6337d01

Alphabetize css attributes.

73a0b0a

Fix dimensions on save-button-tall.

0df308f

Resolve misplacement of -count-above classes.

f293c2a

Add bottom arrow to -bubble-above.

2f0bf17

Remove 'content' attribute.

47cdae6

Add side array to -bubble-beside.

1654b00

googlebot added the cla: yes label Feb 6, 2019

cvializ self-requested a review February 6, 2019 19:25

cvializ reviewed Feb 6, 2019

View reviewed changes

caroqliu added 2 commits February 6, 2019 16:29

Revert examples/pinterest.amp.html

c763b5c

Update -bubble-above and -bubble-beside classes to match fixed dimens…

63fda3c

…ions.

caroqliu requested review from gmajoulet and aghassemi February 7, 2019 14:50

cvializ reviewed Feb 7, 2019

View reviewed changes

Whitelist renamed file save-button.js

0fc3417

amp-bundle-size bot requested a review from danielrozenberg February 7, 2019 16:02

caroqliu added 4 commits February 7, 2019 11:18

Update SVG data-uri to be encoded as utf-8.

a7ad9f0

Refactor SVG data uri to remove inline style tags.

61f245f

Refactor gray color classes to display the logo on Save Buttons in gr…

accc91f

…ayscale.

Revert changes to gray logo -- Realized this is not allowed in brand …

5a30ebd

…guidelines.

caroqliu added 3 commits February 7, 2019 13:42

Replace SVG inline data with converted base64 data to restore a white…

a330331

… border around the Pinterest logo.

Default button color to red instead of gray.

cbc9253

Update PinWidget.js (embedded pins) to show a 'Save' button instead o…

e78f422

…f a 'Pin It' button.

cvializ reviewed Feb 7, 2019

View reviewed changes

extensions/amp-pinterest/0.1/save-button.js Outdated Show resolved Hide resolved

cvializ approved these changes Feb 7, 2019

View reviewed changes

caroqliu added 2 commits February 7, 2019 17:04

Update styling for gray buttons to emulate bubble styling.

70bb4f3

Revert changing to logging request.

a490736

caroqliu mentioned this pull request Feb 7, 2019

🐛Change references from 'Pin It' to 'Save' in amp-pinterest. #20689

Closed

Revert change to logging request.

0d37765

cvializ merged commit 4cd12f1 into ampproject:master Feb 8, 2019

caroqliu deleted the pinterest branch February 8, 2019 18:49

caroqliu restored the pinterest branch February 8, 2019 18:49

caroqliu deleted the pinterest branch February 8, 2019 18:49

caroqliu mentioned this pull request Feb 11, 2019

🖍Clean up amp-pinterest button styling #20784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛Modify amp-pinterest.css from displaying "Pin It" to "Save" as a button #20706

🐛Modify amp-pinterest.css from displaying "Pin It" to "Save" as a button #20706

caroqliu commented Feb 6, 2019 •

edited

cvializ Feb 6, 2019

caroqliu Feb 6, 2019

cvializ Feb 7, 2019

cvializ Feb 7, 2019

caroqliu Feb 7, 2019

cvializ Feb 7, 2019

aghassemi Feb 7, 2019 •

edited

caroqliu Feb 7, 2019

caroqliu Feb 7, 2019

cvializ left a comment •

edited

cvializ Feb 7, 2019

caroqliu Feb 7, 2019

cvializ commented Feb 7, 2019 •

edited

caroqliu commented Feb 7, 2019

cvializ commented Feb 7, 2019

danielrozenberg commented Feb 7, 2019

caroqliu commented Feb 7, 2019

cvializ commented Feb 7, 2019

cvializ left a comment

cvializ commented Feb 8, 2019

caroqliu commented Feb 8, 2019

cvializ commented Feb 8, 2019

🐛Modify amp-pinterest.css from displaying "Pin It" to "Save" as a button #20706

🐛Modify amp-pinterest.css from displaying "Pin It" to "Save" as a button #20706

Conversation

caroqliu commented Feb 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ commented Feb 7, 2019 • edited

caroqliu commented Feb 7, 2019

cvializ commented Feb 7, 2019

danielrozenberg commented Feb 7, 2019

caroqliu commented Feb 7, 2019

cvializ commented Feb 7, 2019

cvializ left a comment

Choose a reason for hiding this comment

cvializ commented Feb 8, 2019

caroqliu commented Feb 8, 2019

cvializ commented Feb 8, 2019

caroqliu commented Feb 6, 2019 •

edited

aghassemi Feb 7, 2019 •

edited

cvializ left a comment •

edited

cvializ commented Feb 7, 2019 •

edited