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

Update Facebook logo, and add .DS_Store to gitignore #94

Merged
merged 2 commits into from
Aug 29, 2019

Conversation

jasmussen
Copy link
Member

This PR does two things.

  1. It updates the Facebook logo to respect new brand guidelines from https://en.facebookbrand.com.
  2. It adds .DS_Store files to the .gitignore.

The old icon:

Screenshot 2019-08-28 at 10 43 38

The new icon:

Screenshot 2019-08-28 at 10 43 58

Pixels:

Screenshot 2019-08-28 at 10 44 04

This PR does two things.

1. It updates the Facebook logo to respect new brand guidelines from https://en.facebookbrand.com.
2. It adds .DS_Store files to the .gitignore.
@jasmussen jasmussen requested a review from folletto August 28, 2019 08:48
@jasmussen jasmussen self-assigned this Aug 28, 2019
@folletto
Copy link
Contributor

Design wise it's good, but the commit shouldn't update all the PDFs, there should be just one update. I'm not sure what's going on.

I've tried to run it from scratch and I get the same update needed on the master branch if I try to do it with no changes, so there's something I think in the PDF package versioning.

There was a mention by Dave in #93 — probably locking pdfkit to a specific version might help, but I'm not sure which one.

I'm now not sure if we should merge this, and then lock the version, or if it's better to lock the version in a separate PR that updates all the PDFs too, and then we re-do this PR from there. Thoughts?

@jasmussen
Copy link
Member Author

There's no urgency to this pr, so I'll be happy to take a stab at a version tomorrow without the PDFs. Thanks so much for the review!

@folletto
Copy link
Contributor

Ok sounds good, ping me for review if you try the other PR! :D

@jasmussen
Copy link
Member Author

Thank you. I'll also look at the PDFs... Maybe they SHOULD be merged, I suppose running the build process a couple of times and looking if they change can reveal something too. Or maybe I need a fresh npm install! In any case great catch.

@jasmussen
Copy link
Member Author

Okay, tried an npm install and npm run build. And all the PDFs changed again, but where before they were smaller, now they're bigger ¯_(ツ)_/¯.

However after a push, I ran an additional npm run build, and here's what a diff shows:

Screenshot 2019-08-29 at 09 19 18

and another npm run build causes this:

Screenshot 2019-08-29 at 09 20 49

So that's at least stable and consistent, and "just an ID" that's changing.

I don't know why the ID is changing, I don't know if we can toggle it off in the build tools, but at least it's consistent.

I looked at svg-to-pdfkit which is what we're using: https://www.npmjs.com/package/svg-to-pdfkit — there doesn't seem to be a way to keep the ID from changing.

So we now have a choice to make:

  1. Do we merge the PDF files, all of them, because at least now they're consistent? If yes, then this PR is ready to go.
  2. Alternately, I made Update Facebook logo, round 2 #95 which contains only the one updated PDF, plus everything else.

I'm okay with either!

thanks for looking again.

@folletto
Copy link
Contributor

So that's at least stable and consistent, and "just an ID" that's changing.

This is very frustrating, it will make reviews harder for no benefit. I don't want to block the PR to take time to scout how to disable that, so let's go ahead and merge the changes.
Also: I guess this means that the same will happen in Gridicons?

Let's merge this PR, making clear in the merge notes that this changes both the single PDF as well as the build for all the others.

@jasmussen
Copy link
Member Author

This is very frustrating, it will make reviews harder for no benefit.

I didn't look super deep, I just couldn't find a flag to stop the ID from changing. There might still be. I agree, by the way.

Okay, I'll merge this one, and yeah it should affect Gridicons too :|

@jasmussen jasmussen merged commit 160952f into master Aug 29, 2019
@jasmussen jasmussen deleted the update/facebook-logo branch August 29, 2019 13:45
jeherve added a commit to Automattic/jetpack that referenced this pull request Jun 26, 2020
Fixes #15954

This update the icon font and the svg, bringing the changes from this:
Automattic/social-logos#94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants