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

Configure default Twitter summary card type (V2) #225

Merged
merged 11 commits into from
May 3, 2018
Merged

Configure default Twitter summary card type (V2) #225

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
82e8d32
Update usage.md
BlythMeister Aug 8, 2017
2f804fd
Add twitterCardType
BlythMeister Aug 8, 2017
06a77d8
Update logic to use twitter_image_small flag
BlythMeister Aug 8, 2017
803c6e1
Update usage for twitter_image_small
BlythMeister Aug 8, 2017
3890d50
Fix issue in template with twitter_image_small
BlythMeister Aug 8, 2017
728b2e9
Merge branch 'master' into TwitterCard2
benbalter Aug 22, 2017
8835d78
Merge branch 'master' into TwitterCard2
DirtyF Feb 14, 2018
3f6ece6
Update template following code review
BlythMeister Feb 15, 2018
f9a59f6
Update usage following code review
BlythMeister Feb 15, 2018
8497fab
Update twitter config example
BlythMeister Feb 15, 2018
31ee52f
Merge branch 'master' into TwitterCard2
DirtyF May 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/usage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,6 @@ The SEO tag will respect the following YAML front matter if included in a post,
* `image` - URL to an image associated with the post, page, or document (e.g., `/assets/page-pic.jpg`)
* `author` - Page-, post-, or document-specific author information (see below)
* `lang` - Page-, post-, or document-specific language information
* `twitter_image_small` - Page-, post-, or document-specific flag to use small twitter image

*Note:* Front matter defaults can be used for any of the above values as described in advanced usage with an image example.
6 changes: 5 additions & 1 deletion lib/template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@

{% if site.twitter %}
{% if seo_tag.image %}
<meta name="twitter:card" content="summary_large_image" />
{% if page.twitter_image_small or post.twitter_image_small %}
<meta name="twitter:card" content="summary" />
{% else %}
<meta name="twitter:card" content="summary_large_image" />
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@DirtyF DirtyF Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 👍

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