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.

Sign up for GitHub

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

Jump to bottom

showcase metatag images size fix [#14469] #14560

Merged
merged 7 commits into from
Jun 10, 2019
Merged

showcase metatag images size fix [#14469] #14560

merged 7 commits into from
Jun 10, 2019

Conversation

abhijithvijayan
Copy link
Contributor

Description

Replaced the meta tag with the publicURL for the showcase images as the current images
(eg: https://www.gatsbyjs.org/static/cbaf442ed29638ce3775856ede4ac483/d808c/8990f01f5a6d31fce109c16ab078b499.jpg) are cropped improperly.

Related Issues

Fixes #14469

@abhijithvijayan abhijithvijayan requested a review from a team as a code owner June 5, 2019 15:56
@abhijithvijayan
Copy link
Contributor Author

The linting failed in windows.

I will try with Ubuntu

@abhijithvijayan abhijithvijayan force-pushed the fix/showcaseMetaImages branch from 6a41700 to 2669dfa Compare June 5, 2019 16:15
superhawk610
superhawk610 previously approved these changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@superhawk610 superhawk610 left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@freiksenet
Copy link
Contributor

CC @fk

fk
fk reviewed Jun 10, 2019
View reviewed changes
www/src/components/showcase-details.js Outdated
@@ -255,15 +256,13 @@ const ShowcaseDetails = ({ parent, data, isModal, categories }) => (
<meta
property="og:image"
content={`https://www.gatsbyjs.org${
data.sitesYaml.childScreenshot.screenshotFile
.childImageSharp.resize.src
data.sitesYaml.childScreenshot.screenshotFile.publicURL
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @abhijithvijayan! Thanks for the PR! 🤗

I just gave this a spin, and publicURL here will always be undefined, unfortunately. :(

This is because data.sitesYaml is coming in from https://github.com/gatsbyjs/gatsby/blob/master/www/src/templates/template-showcase-details.js#L96-L130. You added publicURL to the StaticQuery that takes care of pulling in the screenshot thumbnails for the previous and next navigation shown (only) in the "showcase site" modal.

We were misleading you in https://github.com/gatsbyjs/gatsby/issues/14469#issuecomment-499029287—apologies for that!

Since I'm at it, I'll take the liberty and push a fix so we can get this merged!

If I was unclear in explaining why this doesn't work as you expected it, maybe my follow-up commits will help; if you have any questions, don't hesitate to ask! And please 🙏 don't let this discourage you from contributing! ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank You

@fk
Copy link
Contributor

fk commented Jun 10, 2019

This

  • adds og:title meta information so that Twitter actually will generate a card when sharing a link to a site in the showcase (and expands the page <title> to match what we do for the "Starters" detail pages, e.g. https://www.gatsbyjs.org/starters/gatsbyjs/gatsby-starter-blog/)
  • adjusts the image query to produce an image with a 1.91:1 aspect ratio instead of a square image
  • now that we are serving an appropiate image, tells Twitter to generate a summary_large_image card

Before:
Bildschirmfoto 2019-06-10 um 23 44 02

👇
After:
Bildschirmfoto 2019-06-10 um 23 26 42

m-allanson
m-allanson approved these changes Jun 10, 2019
View reviewed changes
Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

Thanks @abhijithvijayan and @fk 🎉

sidharthachatterjee
sidharthachatterjee approved these changes Jun 10, 2019
View reviewed changes
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Thank you @abhijithvijayan and @fk 🥇

@fk fk merged commit 556b6cf into gatsbyjs:master Jun 10, 2019
@gatsbot
Copy link

gatsbot bot commented Jun 10, 2019

Holy buckets, @abhijithvijayan — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@fk fk mentioned this pull request Jun 10, 2019
Closed
@rlynn523 rlynn523 mentioned this pull request Jun 11, 2019
Merged
mxxk pushed a commit to mxxk/gatsby that referenced this pull request Jun 21, 2019
@abhijithvijayan @mxxk
showcase metatag images size fix [gatsbyjs#14469] (gatsbyjs#14560)
275a91c 
* showcase metatag images size fix [gatsbyjs#14469]

* Rm `publicURL` from static query

* Pull resized image

* Don't create a square thumbnail, but a 1.91:1 landscape crop

* Tell Twitter to use the `summary_large_image` format

* Provide correct dimensions (`og:image:width/height`)

* Add `og:title`, adjust `<title>`…

…following what we do for starter detail pages (e.g. `/starters/gatsbyjs/gatsby-starter-blog/`)
johno pushed a commit to johno/gatsby that referenced this pull request Jul 17, 2019
@abhijithvijayan @johno
showcase metatag images size fix [gatsbyjs#14469] (gatsbyjs#14560)
623dc1a 
* showcase metatag images size fix [gatsbyjs#14469]

* Rm `publicURL` from static query

* Pull resized image

* Don't create a square thumbnail, but a 1.91:1 landscape crop

* Tell Twitter to use the `summary_large_image` format

* Provide correct dimensions (`og:image:width/height`)

* Add `og:title`, adjust `<title>`…

…following what we do for starter detail pages (e.g. `/starters/gatsbyjs/gatsby-starter-blog/`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fk fk fk left review comments

@sidharthachatterjee sidharthachatterjee sidharthachatterjee approved these changes

@m-allanson m-allanson m-allanson approved these changes

@superhawk610 superhawk610 superhawk610 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[www/showcase] "Showcase" entry uses square image as meta twitter:image
6 participants
@abhijithvijayan @freiksenet @fk @m-allanson @sidharthachatterjee @superhawk610