-
Notifications
You must be signed in to change notification settings - Fork 296
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
Configure default Twitter summary card type #222
Conversation
Would be great to have this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Any other objections @pathawks @DirtyF or @benbalter ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objection, except about the naming, don't forget to add documentation if this is accepted.
lib/template.html
Outdated
@@ -51,7 +51,11 @@ | |||
|
|||
{% if site.twitter %} | |||
{% if seo_tag.image %} | |||
<meta name="twitter:card" content="summary_large_image" /> | |||
{% if site.twitter.smallSummary %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just naming this twitter.summary
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, perhaps something like <meta name="twitter:card" content="{{ site.twitter.card | default: "summary_large_image" }}" />
may be more intuitive to end users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea!
Is there a way to do a double default.
E.g.
Page.twitter.card
Then
Site.twitter.card
Then
Summary large image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! You can chain defaults, e.g., | default: page.twitter.card | default: site.twitter.card...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, leave this with me...I like that 👍
Updates made, also given option for a site level image. |
Disclaimer: I'm not sure how to test these changes locally, so it is in untested. I did also update the docs as part of this pr |
I have just realised this is now over complex and could be done using defaults. |
Add ability at site level to not use the large twitter summary
Current rather yucky workaround is doing this:
fix #84