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

Use the site absolute url for twitter:image #285

Merged
merged 6 commits into from
Nov 20, 2018

Conversation

xerial
Copy link
Contributor

@xerial xerial commented Nov 18, 2018

#283 didn't truly fix the issue because Twitter requires absolute URLs for images.

This PR adds a new configuration micrositeUrl when we need to specify the site absolute url.

Alternative approaches:

  • Setting site.url. But if we set this value in _config.yml, it makes difficult to see local images updates because there are many templates using {{site.url}} (e.g., favicon URLs), so all icon images refer remote server URLs.
  • Using a twitter-card specific configuration (e.g., micrositeTwitterCardImageAbsoluteUrl)

With this fix, the card validator https://cards-dev.twitter.com/validator of Twitter icon can properly show the card image (/image/poster.png):
image

@manosbatsis
Copy link

Apologies for hijacking, but would it be possible to also fix og:image for linkedin in this PR? Not sure if an absolute URL is also needed here or if the format of og:image is not exactly correct - perhaps both name and property attributes are required - see details from linkedin post inspector bellow:

linkedinimage

@xerial
Copy link
Contributor Author

xerial commented Nov 19, 2018

@manosbatsis Added fixes for Linked-in as well. I've added properties for og:image and og:title properties, and confirmed it's working well: https://www.linkedin.com/post-inspector/inspect/https:%2F%2Fwvlet.org%2Fairframe%2F

@manosbatsis
Copy link

That's fantastic, many thanks @xerial - looking forward to 0.7.26!

@calvellido calvellido self-requested a review November 19, 2018 09:14
Copy link
Contributor

@calvellido calvellido left a comment

Choose a reason for hiding this comment

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

Nice @xerial! Would you mind to add that new config parameter to the docs? Basically this file: settings.md

A simple description on why that parameter might be useful would suffice.

@xerial
Copy link
Contributor Author

xerial commented Nov 19, 2018

OK!

@xerial
Copy link
Contributor Author

xerial commented Nov 19, 2018

@calvellido Added a note to settings.md

Copy link
Contributor

@calvellido calvellido left a comment

Choose a reason for hiding this comment

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

👏 👏 👏

@calvellido calvellido merged commit a4eda74 into 47degrees:master Nov 20, 2018
@calvellido
Copy link
Contributor

Just released 0.7.26 including these changes, thanks so much @xerial!

@manosbatsis
Copy link

Grand, thanks guys @calvellido and @xerial

@manosbatsis
Copy link

Hm, still can't see the new version on central after six hours or so. @calvellido this normal?

@juanpedromoreno
Copy link
Member

It failed: https://travis-ci.org/47deg/sbt-microsites/jobs/457450111#L1488

I'll fix it.

@juanpedromoreno
Copy link
Member

0.7.26 should be now on its way to Maven central.

@manosbatsis
Copy link

@juanpedromoreno confirmed, cheers

@calvellido
Copy link
Contributor

Thanks @juanpedromoreno! 😘

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.

4 participants