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

Improve logo for Twitter cards #3304

Merged
merged 6 commits into from
Mar 20, 2018
Merged

Improve logo for Twitter cards #3304

merged 6 commits into from
Mar 20, 2018

Conversation

jmuzsik
Copy link

@jmuzsik jmuzsik commented Mar 17, 2018

In reference to #3112, literally solely changed the image used as twitter does not support svg images.

@jmuzsik
Copy link
Author

jmuzsik commented Mar 17, 2018

I'll work on fixing that test, thought it would be a quick one liner fix, I should have tested previously.

@brainwane brainwane changed the title Closes https://github.com/pypa/warehouse/issues/3112 Improve logo for Twitter cards Mar 18, 2018
@jmuzsik
Copy link
Author

jmuzsik commented Mar 19, 2018

Hi, this is the problem:
screen shot 2018-03-18 at 9 17 13 pm

Image is not cache busted. This is, what I believe to be the solution as the tests pass when I add it:

screen shot 2018-03-18 at 9 12 04 pm

but I cannot push changes to the manifest.json, so I suppose I'd need to wait for that to happen.

@nlhkabu nlhkabu requested a review from di March 19, 2018 06:33
@di
Copy link
Member

di commented Mar 19, 2018

@jmuzsik Yeah, sorry for the confusion. The manifest.json file is generated automatically when compiling our static assets. Most of our static assets live in a separate private repository for... reasons, so the actual assets in ./warehouse/static/images are just placeholders.

You should just create a dummy image, name it twitter.jpg, and put it in that directory for this PR.

I went ahead and created one for you to save you the trouble:

twitter

@jmuzsik
Copy link
Author

jmuzsik commented Mar 20, 2018

For one reason or another, jpg did not cache in the manifest.json so I converted the image to png, in which case it worked.

@di
Copy link
Member

di commented Mar 20, 2018

@jmuzsik Hmm, the actual image is currently a .jpg, so we'll either need to either a) figure out why it didn't work or b) change it.

Was there an error? Or did it just not appear in the manifest? I'm guessing it's because the extension is not listed in the includeFilesInManifest argument in Gulpfile.js, we might just need to add it.

Probably the best to do this for the future when this might happen again, unless there is some reason why we can't use .jpgs (not totally sure why @nlhkabu made the original image a .jpg, perhaps she can chime in here).

@jmuzsik
Copy link
Author

jmuzsik commented Mar 20, 2018

Here is a couple images, for whatever reason, jpg never got into the manifest.json. It doesn't exactly make sense, I was quite confused for a bit of time not knowing what was going on.

And no error, only the tests failed because of the same caching problem, so likely what you are thinking. I'll do that as well, add the extension to gulpFile

@jmuzsik
Copy link
Author

jmuzsik commented Mar 20, 2018

Tried altering the places that appeared reasonable to add jpg to but still it does not work. Cache busting error still occurs when running the tests because it does not go into the manifest.json. The jpg always minified though.

screen shot 2018-03-20 at 3 31 49 pm

screen shot 2018-03-20 at 3 32 04 pm

screen shot 2018-03-20 at 3 32 20 pm

screen shot 2018-03-20 at 2 58 19 pm

@di
Copy link
Member

di commented Mar 20, 2018

@jmuzsik Sorry if this is a little confusing. Are you checking the manifest.json in your local repo, or inside the static container? If the former, are you re-running gulp dist after making the changes to the gulpfile?

I added twitter.jpg to warehouse/static/images/, made the following changes:

diff --git a/Gulpfile.babel.js b/Gulpfile.babel.js
index e77754c..298ad58 100644
--- a/Gulpfile.babel.js
+++ b/Gulpfile.babel.js
@@ -215,6 +215,7 @@ gulp.task("dist:manifest", () => {
         ".ttf",
         ".otf",
         ".png",
+        ".jpg",
         ".ico",
         ".js",
       ],
@@ -246,6 +247,7 @@ gulp.task("dist:compress:br:generic", () => {
     path.join(distPath, "fonts", "*.svg"),

     path.join(distPath, "images", "*.png"),
+    path.join(distPath, "images", "*.jpg"),
     path.join(distPath, "images", "*.svg"),
     path.join(distPath, "images", "*.ico"),
   ];

running gulp dist correctly put twitter.jpg in manifest.json:

{
  "css/font-awesome.css": "css/font-awesome.628aab96.css",
  "css/warehouse.css": "css/warehouse.468a3572.css",
  "css/font-awesome.css.map": "css/font-awesome.css.4741d860.map",
  "css/warehouse.css.map": "css/warehouse.css.8db81ae8.map",
  "fonts/FontAwesome.otf": "fonts/FontAwesome.b5744b64.otf",
  "fonts/fontawesome-webfont.eot": "fonts/fontawesome-webfont.1fb8a705.eot",
  "fonts/fontawesome-webfont.svg": "fonts/fontawesome-webfont.912ec66d.svg",
  "fonts/fontawesome-webfont.ttf": "fonts/fontawesome-webfont.e1dd9adb.ttf",
  "fonts/fontawesome-webfont.woff": "fonts/fontawesome-webfont.a7bbeb58.woff",
  "fonts/fontawesome-webfont.woff2": "fonts/fontawesome-webfont.68560c51.woff2",
  "js/warehouse.js": "js/warehouse.b60a2f5f.js",
  "js/warehouse.js.map": "js/warehouse.js.0cd523c9.map",
  "js/vendor/zxcvbn.js": "js/vendor/zxcvbn.9cf6916d.js",
  "images/blue-cube-small.png": "images/blue-cube-small.c34cf0bf.png",
  "images/blue-cube.svg": "images/blue-cube.e6165d35.svg",
  "images/favicon.ico": "images/favicon.6a76275d.ico",
  "images/history-line.png": "images/history-line.ded09064.png",
  "images/logo-large.svg": "images/logo-large.87e8a3ef.svg",
  "images/logo-small.svg": "images/logo-small.972290d5.svg",
  "images/twitter.jpg": "images/twitter.21ac1c93.jpg",
  "images/white-cube-small.png": "images/white-cube-small.95381ba2.png",
  "images/white-cube.png": "images/white-cube.9f6d32b9.png",
  "images/white-cube.svg": "images/white-cube.8c3a6fe9.svg"
}

@nlhkabu
Copy link
Contributor

nlhkabu commented Mar 20, 2018

@di - no particular reason to use a .jpg. It was just the example I found in the twitter docs. If it makes any difference, I am happy to provide a .png instead?

according to the docs:

JPG, PNG, WEBP and GIF formats are supported

@di
Copy link
Member

di commented Mar 20, 2018

@nlhkabu I think it's fine but we'll see if @jmuzsik is able to get this resolved on his end with the JPG.

@jmuzsik
Copy link
Author

jmuzsik commented Mar 20, 2018

Ah, sorry for taking up some of your time for this beginner problem. Thanks for the info. I was not running gulp dist, but thought that the automatic recreation that occurred in make serve was doing this for me already.

Thanks very much!

@di
Copy link
Member

di commented Mar 20, 2018

@jmuzsik Could you make the image in this PR the full-resolution image I included in the comment above? As it is, the placeholder image is too small to actually be accepted by Twitter, so testing this locally in the future with something like ngrok would fail.

@di di merged commit 088f648 into pypi:master Mar 20, 2018
@di
Copy link
Member

di commented Mar 20, 2018

Thanks @jmuzsik!

@jmuzsik
Copy link
Author

jmuzsik commented Mar 20, 2018

Thanks for all the help!

@nlhkabu
Copy link
Contributor

nlhkabu commented Mar 21, 2018

🎉

@jmuzsik jmuzsik deleted the twitter_metadata branch March 21, 2018 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants