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

🖍 [bento][social-share] Update outdated social icons and colors #29867

Merged
merged 8 commits into from
Sep 23, 2020

Conversation

krdwan
Copy link
Contributor

@krdwan krdwan commented Aug 17, 2020

Updated SVGs and background colors:

Visual of the new icons can be found here:
https://docs.google.com/presentation/d/1WmdMNQ0p06ewIby2sIHfZE_2_kwuL4WLrJYjVmZo13Y/edit?usp=sharing

Actual SVG code can be found here:
https://drive.google.com/drive/folders/1l4SbiPL_b8sNPI1OkuoGp4LQApvr_c2-?usp=sharing

Default Background colors are summarized here:

Icon New Background Color Previous Background Color
twitter #1da1f2 #1da1f2
facebook #1877f2 #32529f
pinterest #e60023 #e60023
linkedin #0a66c2 #0077b5
tumblr "#1935" #3c5a77
email #000000 #000000
whatsapp #25d366 #25d366
line #25d366 #52b448
sms #000000 #ca2b63
system #000000 #000000

@dvoytenko
Copy link
Contributor

The smallish big question here: if we're providing our own SVGs here, we should do it in our own source tree - not in third_party/someone-else-s-stuff.

@krdwan
Copy link
Contributor Author

krdwan commented Aug 19, 2020

The smallish big question here: if we're providing our own SVGs here, we should do it in our own source tree - not in third_party/someone-else-s-stuff.

Makes sense, will update and move the components to within Social Share!

@jridgewell jridgewell added this to In progress in Bento Aug 21, 2020
@krdwan
Copy link
Contributor Author

krdwan commented Sep 16, 2020

Changes

  1. Line background color updated from 25d366 to 00B900
  2. Move svg file into extensions/amp-social-share
  3. FB and Pinterest SVGs updated to circular. Background set to white, foreground set to a specific color.
Icon New Background Color Previous Background Color
twitter #1da1f2 #1da1f2
facebook #1877f2 #32529f
pinterest #e60023 #e60023
linkedin #0a66c2 #0077b5
tumblr "#1935" #3c5a77
email #000000 #000000
whatsapp #25d366 #25d366
line #00B900 #52b448
sms #000000 #ca2b63
system #000000 #000000

@krdwan
Copy link
Contributor Author

krdwan commented Sep 16, 2020

Removed directory of old SVG icons.

@@ -1,17 +0,0 @@
URL: http://codepen.io/anon/pen/KVZNGp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems premature? Isn't 0.1 still using these resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, thanks for the reminder! I will revert this part!

Bento automation moved this from In progress to Reviewer approved Sep 17, 2020
@krdwan
Copy link
Contributor Author

krdwan commented Sep 21, 2020

Reversed FB foreground and background colors. Final set of colors in the table shown below

Icon New Foreground Previous Foreground New Background Previous Background
twitter #ffffff #ffffff #1da1f2 #1da1f2
facebook #ffffff #ffffff #1877f2 #32529f
pinterest #e60023 #ffffff #ffffff #e60023
linkedin #ffffff #ffffff #0a66c2 #0077b5
tumblr #ffffff #ffffff #1935 #3c5a77
email #ffffff #ffffff #000000 #000000
whatsapp #ffffff #ffffff #25d366 #25d366
line #ffffff #ffffff #00b900 #52b448
sms #ffffff #ffffff #000000 #ca2b63
system #ffffff #ffffff #000000 #000000

@krdwan krdwan marked this pull request as ready for review September 21, 2020 19:33
@@ -30,7 +30,6 @@ const COMMON_GLOBS = [
'third_party/inputmask/**/*.js',
'third_party/mustache/**/*.js',
'third_party/react-dates/bundle.js',
'third_party/optimized-svg-icons/social-share-svgs.js',
'third_party/set-dom/set-dom.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dvoytenko Is there someone you would recommend to review this file? I changed it to pass Travis, but am not very familiar with this part of the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. My review should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

@krdwan
Copy link
Contributor Author

krdwan commented Sep 22, 2020

Final Colors

Latest commit reverses facebook foreground and background colors. Final colors noted in table below

Icon New Foreground Previous Foreground New Background Previous Background
twitter #ffffff #ffffff #1da1f2 #1da1f2
facebook #1877f2 #ffffff #ffffff #32529f
pinterest #e60023 #ffffff #ffffff #e60023
linkedin #ffffff #ffffff #0a66c2 #0077b5
tumblr #ffffff #ffffff #1935 #3c5a77
email #ffffff #ffffff #000000 #000000
whatsapp #ffffff #ffffff #25d366 #25d366
line #ffffff #ffffff #00b900 #52b448
sms #ffffff #ffffff #000000 #ca2b63
system #ffffff #ffffff #000000 #000000

Screenshot of default colors
image

Screenshot of styled colors (yellow foreground, blue background)
image

fyi: @nainar

@krdwan krdwan merged commit 8aa2883 into ampproject:master Sep 23, 2020
Bento automation moved this from Reviewer approved to Done Sep 23, 2020
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
…roject#29867)

* Update icons and colors

* Update icons

* Delete old svg icons directory

* Revert "Delete old svg icons directory"

This reverts commit 86d2fbd.

* Reverse foreground and background colors for FB

* Remove third party dependency check in config file

* Remove svgs from source.js blob file

* Reverse foreground and background colors for Facebook
@@ -150,7 +149,6 @@ const CLOSURE_SRC_GLOBS = [
*/
const THIRD_PARTY_TRANSFORM_GLOBS = [
// JSX syntax should undergo usual transforms
'third_party/optimized-svg-icons/social-share-svgs.js',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krdwan Curious about why THIRD_PARTY_TRANSFORM_GLOBS was left here an empty list. Any reason to keep it around?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rsimha I wasn't sure if this was a generic part of the AMP build system and that it might be updated to be non-empty in the future. From the perspective of this PR and the social-share component, there's no reason to keep it around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bento
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants