Issue 5329 - Provide a template for social media meta data in website-defaults

includes/meta/social.tmpl

+{% from "macros/meta-tag" import meta_tag %}
+
 {% set social_media_meta = [
-  ('og:url', 

[juliandoucette, 2018/01/30 04:04:40]

NIT: I'm really not digging this 
('multi',
 line
 style()),

Looks like a mess to me :/

[ire, 2018/01/30 08:19:08]

I don't think it's very ideal either, but I also think putting everything on one
line is: 1) difficult to read 2) frequently longer than 80 characters

[juliandoucette, 2018/01/31 19:03:22]

Acknowledged.

-    og_url, 
+  ('og:url',
+    og_url,
     get_canonical_url(page)),
-  ('og:type', 

[juliandoucette, 2018/01/30 04:04:40]

NIT: I think the default is website and we don't need it unless it's not
'website' (which it may not ever be). Please double check, as it doesn't hurt
very much to include this anyway.

[ire, 2018/01/30 08:19:08]

I doesn't seem that this data is implied without being specified. See doc -
http://ogp.me/#types

[juliandoucette, 2018/01/31 19:03:21]

Acknowledged.

-    og_type, 
+  ('og:type',
+    og_type,
     'website'),
-  ('og:site_name', 
-    og_site_name, 
-    get_string('name', 'site') if has_string('name', 'site') else None),

[ire, 2018/01/26 11:33:59]

I changed this to use the same site name string as the standard metadata

[juliandoucette, 2018/01/30 04:04:39]

NIT: `condition and output` can replace `output if condition else none`

[juliandoucette, 2018/01/30 04:04:40]

Acknowledged.

[ire, 2018/01/30 08:19:08]

Are you saying a valid way to write this is `has_string('name', 'site') and
get_string('name', 'site')` ?

[juliandoucette, 2018/01/31 19:03:21]

Yes. But apparently you can also just drop the `else None`. This seems to
working Jinja and not in pure python.

[ire, 2018/02/01 09:07:01]

The line `has_string('name', 'site') and get_string('name', 'site')` throws an
error

I dropped the `else None`

-  ('og:title', 
-    og_title, 

[ire, 2018/01/26 11:33:59]

Should `og_title` be translated as well (since title is)? Same for
og_description

[juliandoucette, 2018/01/30 04:04:40]

Yes.

[ire, 2018/01/30 08:19:08]

Done.

-    title | translate('title', 'Meta data') if title else None),
-  ('og:description', 
-    og_description, 
-    description | translate('page-description', 'Meta data') if description else None),
-  ('og:image', 
-    og_image, 
+  ('og:site_name',
+    og_site_name,
+    get_string('name', 'site') if has_string('name', 'site')),
+  ('og:title',
+    og_title | translate('og-title', 'Meta data') if og_title,
+    title | translate('title', 'Meta data') if title),
+  ('og:description',
+    og_description | translate('og-description', 'Meta data') if og_description,
+    description | translate('page-description', 'Meta data') if description),
+  ('og:image',
+    og_image,
     featured_img_url),
-  ('og:image:alt', 
-    og_image_alt, 
+  ('og:image:alt',
+    og_image_alt,
     featured_img_alt),
-  ('og:locale', 
-    og_locale, 
+  ('og:locale',
+    og_locale,
     locale | to_og_locale),
-  
-  ('twitter:site', 
-    twitter_site, 
-    config.get('social', 'twitter') if config.has_option('social', 'twitter') else '@eyeo'),

[ire, 2018/01/26 11:33:59]

I think the site's twitter username, facebook id, and pinterest ID should be
able to be set globally as they are more likely to not change across pages so I
added this global option in site config

[juliandoucette, 2018/01/30 04:04:40]

NIT: Long line

[juliandoucette, 2018/01/30 04:04:40]

Acknowledged.

[ire, 2018/01/30 08:19:08]

I can't think of any better way to write this than would be shorter :/

[juliandoucette, 2018/01/31 19:03:21]

Acknowledged.

-  ('twitter:card', 
-    twitter_card, 
+
+  ('twitter:site',
+    '@' + twitter_site if twitter_site,
+    '@' + config.get('social', 'twitter') if config.has_option('social', 'twitter') else '@eyeo'),
+  ('twitter:card',
+    twitter_card,
     'summary_large_image' if og_image or twitter_image or featured_img_url else 'summary'),
-  ('twitter:image', 

[juliandoucette, 2018/01/30 04:04:40]

NIT: We may not want to add og:image and twitter:image for the sake of
simplicity in the first version - as we have not recieved facebook and twitter
specific images in the past.

[ire, 2018/01/30 08:19:08]

Ack. But there's no reason to exclude it now, is there?

[juliandoucette, 2018/01/31 19:03:21]

We should not maintain code that we do not and probably will not use.

[ire, 2018/02/01 09:07:01]

It will be used for pages that already have a featured image, since that is the
fallback. e.g. the acceptableads.com blog posts.

-    twitter_image, 
+  ('twitter:image',
+    twitter_image,
     featured_img_url),
-  ('twitter:image:alt', 
-    twitter_image_alt, 
+  ('twitter:image:alt',
+    twitter_image_alt,
     featured_img_alt),
 
-  ('fb:app_id', 
-    facebook_id, 
-    config.get('social', 'facebook_id') if config.has_option('social', 'facebook_id') else None),
+  ('fb:app_id',
+    facebook_id,
+    config.get('social', 'facebook_id') if config.has_option('social', 'facebook_id')),
 
-  ('p:domain_verify', 
-    pinterest_id, 
-    config.has_option('social', 'pinterest_id') if config.has_option('social', 'pinterest_id') else None),
+  ('p:domain_verify',
+    pinterest_id,
+    config.has_option('social', 'pinterest_id') if config.has_option('social', 'pinterest_id')),
 ] %}
-
-{% macro meta_tag(property, content, defaultContent=None) -%}

[juliandoucette, 2018/01/30 04:04:40]

NIT: We could use this macro elsewhere if you separate it (into a separate
file).

[ire, 2018/01/30 08:19:07]

Done.

-  {% if content or defaultContent %}
-    <meta property='{{ property }}' content='{{ content or defaultContent }}'>
-  {% endif %}
-{%- endmacro %}
 
 {% for property, content, defaultContent in social_media_meta %}

[juliandoucette, 2018/01/30 04:04:40]

NIT: You could use a for in loop here instead of defining a list above e.g.

for property, content, default in [
  ('og:site_name', og_site_name, has_string('name', 'site') and
get_string('name', 'site')),
]

[ire, 2018/01/30 08:19:08]

That seems more messy to me. I think it's better to separate the defining of the
content from where it's being used, particularly where the content is so long.

[juliandoucette, 2018/01/31 19:03:21]

Acknowledged.

   {{ meta_tag(property, content, defaultContent) }}
 {% endfor %}
-
-{% for alternate_locale in available_locales %}
-  {% if alternate_locale != locale %}
-    {{ meta_tag(('og:locale:alternate', alternate_locale | to_og_locale)) }}

[juliandoucette, 2018/01/30 04:04:40]

Detail: This implementation is correct but it doesn't actually do anything
useful to/for any existing social networks that I know about (I tested and filed
bugs with facebook before. The result was that they fixed their share debugger
to not work just like shares didn't work - the way I wanted them to).

The same functionality can be implemented for Google at least using a link
element with hreflang and rel="alternate" (not sure if that should be considered
standard meta data or social - I'm leaning towards standard.).

[ire, 2018/01/30 08:19:08]

Ack. I think you're right it should go into standard. I'll remove it from this
review and we can re-add it in a separate issue

-  {% endif %}
-{% endfor %}