Configure default Twitter summary card type (V2) #225

BlythMeister · 2017-08-08T09:15:20Z

Update to #222 with cleaner history.

Original details:

Add ability at site level to not use the large twitter summary

Current rather yucky workaround is doing this:

{% capture seo %}
{% seo %}
{% endcapture %}

{{ seo | replace: "summary_large_image", "summary" }}

fix #84

BlythMeister · 2017-08-08T09:18:30Z

Sorry for all the pr open/close.
Had a right nightmare

BlythMeister · 2017-08-08T09:34:32Z

Have seen this done with a twitter_image_small flag set to true or false.
What's you opinion on that instead?

BlythMeister · 2017-08-08T18:04:20Z

example usage:

workaround in practice: https://github.com/BlythMeister/blyth.me.uk/blob/master/_includes/head.html#L26
default config: https://github.com/BlythMeister/blyth.me.uk/blob/master/_config.yml#L39

BlythMeister · 2017-08-08T18:16:06Z

I have no idea why this build is failing....can someone help?

pathawks · 2017-08-08T18:22:14Z

Failing test looks unrelated.

BlythMeister · 2017-08-08T18:23:21Z

Weird for it to suddenly start failing though, master looks to be passing.

pathawks · 2017-08-09T04:03:39Z

This is the test that is failing. I don't know how it could be related to the changes in this PR.

/cc: @benbalter

benbalter · 2017-08-09T13:40:09Z

The failing test looks like a change in either Addressable or Jekyll's invalid URL handling behavior and can likely be removed, either here, or via a separate PR and then merge in master.

BlythMeister · 2017-08-09T14:29:26Z

Changing liquid and documentation I can do.
Ruby...not so much 😋

pathawks · 2017-08-09T20:56:03Z

Changing liquid and documentation I can do.
Ruby...not so much

For what it's worth, that was exactly where I was when I started hacking on Jekyll.

That said, if I have time tonight I will look into modifying/dropping the failing test 👍

benbalter · 2017-08-22T15:54:30Z

The failing test looks like a change in either Addressable or Jekyll's invalid URL handling behavior and can likely be removed, either here, or via a separate PR and then merge in master.

Fixed via #228

benbalter · 2017-08-22T17:10:35Z

lib/template.html

+      <meta name="twitter:card" content="summary" />
+    {% else %}
+      <meta name="twitter:card" content="summary_large_image" />
+    {% endif %}


Per the discussion over in #222 (comment), would it make more sense to implement this as something like:

<meta name="twitter:card" content="{{ page.twitter.card | default: site.twitter.card | default: "summary_large_image" }}" />

That way if Twitter adds a "medium" card next year, we're still covered.

I did try this and whilst I agree it does add future scope, the usage in the consuming site doesn't look/feel as nice as a true/false.

If adding as discussed with defaults is the only way to get this in, then I guess I concede, but imo, code should be implemented for the current scope, not all possible future scope 🙂

Not strongly opinionated. If others feel strongly one way or the other, glad to support the consensus.

My other theory is, with this, users don't need to know the names Twitter use, they can see in the docs here there is a true/false to make it small.

There are already other types of cards (app and player for now): https://dev.twitter.com/cards/types. We might as well support those.

App and player cards require other meta data info rather than just the image, do there is quite a bit more effort required.
For a simple image based card which is what this section has, there is just small or big image.

Having been 6 months on this...is the only way to get this in to force the user to put the exact card type into their front matter?

To repeat why I went with this approach, Twitter has 2 card types that would work with the rest of the meta this plugin adds.
By using a Boolean option, the user doesn't need to know the exact twitter card names (which is one plus for this whole plugin, the user doesn't need to know all the meta tags)

I would appreciate a definite change to get in, or a merge as is so this can move forward please.

The boolean seems clunky. Ie: Why twitter_image_small: true instead of twitter_image_large: false?

Let's go with twitter.card: summary_large_image 👍

pathawks · 2018-02-14T19:21:41Z

lib/template.html

+      <meta name="twitter:card" content="summary" />
+    {% else %}
+      <meta name="twitter:card" content="summary_large_image" />
+    {% endif %}


The boolean seems clunky. Ie: Why twitter_image_small: true instead of twitter_image_large: false?

Let's go with twitter.card: summary_large_image 👍

pathawks · 2018-02-14T19:22:42Z

@BlythMeister Sorry this has sat for so long. Ping me when you've made the changes and I will review.

BlythMeister · 2018-02-15T09:23:44Z

@pathawks updates made as requested.

benbalter

We might want to change the behavior down the line if Twitter offers additional card types, but 👍 for now.

DirtyF · 2018-02-15T16:21:14Z

docs/usage.md


  ```yml
  twitter:
    username: benbalter
+    card: summary_large_image


What are the other card types available? We could use a value different from the default's one here.

This is so it doesn't use the default value

BlythMeister · 2018-02-15T17:50:09Z

@DirtyF updated to "summary" which is the smaller card

Remove Facebook app ID Applied workaround for twitter:card value (see. jekyll/jekyll-seo-tag#225)

BlythMeister · 2018-05-02T17:30:05Z

@DirtyF @benbalter @pathawks can this be put in now?

pathawks · 2018-05-03T00:04:17Z

Awesome. Thanks for all your work on this! 👍🍻

@jekyllbot: +minor

BlythMeister added 2 commits August 8, 2017 10:12

Update usage.md

82e8d32

Add twitterCardType

2f804fd

BlythMeister added 3 commits August 8, 2017 18:46

Update logic to use twitter_image_small flag

06a77d8

Update usage for twitter_image_small

803c6e1

Fix issue in template with twitter_image_small

3890d50

benbalter mentioned this pull request Aug 22, 2017

Fix for image.path causing an invalid url error #228

Merged

Merge branch 'master' into TwitterCard2

728b2e9

benbalter reviewed Aug 22, 2017

View reviewed changes

pathawks requested changes Feb 14, 2018

View reviewed changes

DirtyF and others added 3 commits February 14, 2018 23:16

Merge branch 'master' into TwitterCard2

8835d78

Update template following code review

3f6ece6

Update usage following code review

f9a59f6

pathawks approved these changes Feb 15, 2018

View reviewed changes

benbalter approved these changes Feb 15, 2018

View reviewed changes

DirtyF approved these changes Feb 15, 2018

View reviewed changes

DirtyF reviewed Feb 15, 2018

View reviewed changes

Update twitter config example

8497fab

This is so it doesn't use the default value

grrseguin added a commit to hl2/hl2.com that referenced this pull request Apr 24, 2018

Fix SEO image

da73eb0

Remove Facebook app ID Applied workaround for twitter:card value (see. jekyll/jekyll-seo-tag#225)

Merge branch 'master' into TwitterCard2

31ee52f

jekyllbot merged commit 643c2db into jekyll:master May 3, 2018

jekyllbot added the enhancement label May 3, 2018

jekyllbot added a commit that referenced this pull request May 3, 2018

Update history to reflect merge of #225 [ci skip]

4617e87

set0gut1 mentioned this pull request May 12, 2018

add tests for twitter.card types #289

Merged

jekyll locked and limited conversation to collaborators May 3, 2019

jekyllbot added the frozen-due-to-age label May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure default Twitter summary card type (V2) #225

Configure default Twitter summary card type (V2) #225

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

pathawks commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

pathawks commented Aug 9, 2017

benbalter commented Aug 9, 2017

BlythMeister commented Aug 9, 2017

pathawks commented Aug 9, 2017

benbalter commented Aug 22, 2017

benbalter Aug 22, 2017

BlythMeister Aug 22, 2017

benbalter Aug 22, 2017

BlythMeister Aug 22, 2017

DirtyF Aug 22, 2017 •

edited

Loading

BlythMeister Aug 23, 2017

BlythMeister Feb 14, 2018

pathawks Feb 14, 2018

pathawks Feb 14, 2018

pathawks commented Feb 14, 2018

BlythMeister commented Feb 15, 2018

benbalter left a comment

DirtyF Feb 15, 2018

BlythMeister commented Feb 15, 2018

BlythMeister commented May 2, 2018

pathawks commented May 3, 2018

Configure default Twitter summary card type (V2) #225

Configure default Twitter summary card type (V2) #225

Conversation

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

pathawks commented Aug 8, 2017

BlythMeister commented Aug 8, 2017

pathawks commented Aug 9, 2017

benbalter commented Aug 9, 2017

BlythMeister commented Aug 9, 2017

pathawks commented Aug 9, 2017

benbalter commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirtyF Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pathawks commented Feb 14, 2018

BlythMeister commented Feb 15, 2018

benbalter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlythMeister commented Feb 15, 2018

BlythMeister commented May 2, 2018

pathawks commented May 3, 2018

DirtyF Aug 22, 2017 •

edited

Loading