Issue 29469558: Issue 5329 - Optimize website-boilerplate for sharing on social media networks (Closed)
|
Created:
June 20, 2017, 2:38 p.m. by juliandoucette Modified:
July 7, 2017, 9:34 a.m. CC:
wspee, Lisa Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionNote: I uploaded this to review against website-defaults - as it will probably land in website-defaults despite website-boilerplate in the name. You may checkout website-boilerplate (https://gitlab.com/eyeo/website-boilerplate) and test these includes. But the first priority is to agree on the fields that we would like to support across websites.
Patch Set 1 #Patch Set 2 : Removed shortcut icon #
Total comments: 11
Patch Set 3 : Added og:type #
Total comments: 15
Patch Set 4 : Fixed typos #Patch Set 5 : Separated open graph and twitter fields #
Total comments: 8
MessagesTotal messages: 17
Ready for review. Note: I made all fields optional for the sake of backwards compatibility (so that we can port this to existing websites without breaking them). Also, I documented all fields that should be configured by a developer below the copyright notice.
Added notes. I'm thinking about adding a separate include for articles/blog posts. https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:34: # : Featured image (at least 1200x630, up to 5MB) Note: The dimensions come from facebook and the file size restriction comes from twitter. https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:35: #} Looks like I'm missing og:type. https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:39: {% if twitter_site %} Note: I'm saving twitter:creator for another include specifically for articles (blog posts). https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:42: {% if og_title %} Note: title and description are not redundant because they should be targeted differently. See the descriptions of these fields above (and in standard.tmpl). https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:50: {% else if og_site_image %} Note: This allows us to optionally configure a default image for the site via global. https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:53: {% if twitter_card %} Note: I thought these defaults made sense. https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:60: {% if twitter_image_alt %} Note: It's a shame that open graph doesn't support this. I'm tempted to use og:image:alt anyway. https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:63: {% if host and page %} Note: We currently do not have a way to output the base url via CMS. As a result, I am using an optionally configured global variable "host". https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/stand... includes/meta/standard.tmpl:54: <meta http-equiv="x-ua-compatible" content="ie=edge"> Note: We have had discussions about these meta tags in the past (http-equiv & viewport) and determined that they were necessary for responsiveness on mobile and IE (I think). https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/stand... includes/meta/standard.tmpl:56: {% if title %} Note: I had considered enforcing the optimal format of titles via something like: title - category | site_name. But I decided against it so that we can have a more fine-grained control of the title length.
https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469565/includes/meta/socia... includes/meta/social.tmpl:35: #} On 2017/06/20 14:57:54, juliandoucette wrote: > Looks like I'm missing og:type. Done. Note: "website" is the default according to ogp.me.
Thanks! Suggestion: Should we add the `fb:app_id`? This allows Facebook to tie our Facebook Page to shares of the page. It's mainly useful for additional analytics/insights so not sure if it's a priority https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:19: # # Essential meta data for sharing on social media This list in the comments isn't representative of the values used below. Will there be a separate list/documentation of all the variable names elsewhere? https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:24: # og:site This should be og:site_name https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:29: # NIT: Can you group the `og` and the `twitter` metadata together? So when we read it we're not going back and forth between different types of metadata https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:68: {% endif %} These metadata are missing for twitter cards: twitter:image - The url of the image twitter:title (required) - Card title twitter:description (required) - Card description twitter:creator - This is the author of the page content. As opposed to twitter:site, which is the username for the website See requirements for [Summary Card](https://dev.twitter.com/cards/types/summary) and [Summary Card with Large Image](https://dev.twitter.com/cards/types/summary-large-image) https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/stand... includes/meta/standard.tmpl:24: # : Primary Keeyword - Secondary Keyword | Brand Name Typo in "Keeyword"
Thanks Ire :) - See answers below. https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:1: {# > Suggestion: Should we add the `fb:app_id`? This allows Facebook to tie our > Facebook Page to shares of the page. It's mainly useful for additional > analytics/insights so not sure if it's a priority Does fb:app_id apply to pages? https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:19: # # Essential meta data for sharing on social media On 2017/06/21 11:39:29, ire wrote: > This list in the comments isn't representative of the values used below. Will > there be a separate list/documentation of all the variable names elsewhere? I excluded fields that I automated (e.g.locale). https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:24: # og:site On 2017/06/21 11:39:29, ire wrote: > This should be og:site_name Done. Good catch. https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:29: # On 2017/06/21 11:39:29, ire wrote: > NIT: Can you group the `og` and the `twitter` metadata together? So when we read > it we're not going back and forth between different types of metadata Acknowledged. I grouped them categorically. e.g. twitter:site after og:site, twitter:image:alt after og:image. I'll take your suggestion. https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:47: {% endif %} Note: There is another proprietary field for associating pintrist accounts with websites. I haven't included it here yet. https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:68: {% endif %} On 2017/06/21 11:39:29, ire wrote: > twitter:image - The url of the image Falls back to og:image > twitter:title (required) - Card title Falls back to og:title > twitter:description (required) - Card description Falls back to og:description > twitter:creator - This is the author of the page content. As opposed to Ack. I intend to create a separate include for articles/blog posts where things like twitter:creator and og:article:author will be relevant. I don't think that a twitter:creator is needed on other pages. Do you agree? > See requirements for [Summary Card](https://dev.twitter.com/cards/types/summary) > and [Summary Card with Large > Image](https://dev.twitter.com/cards/types/summary-large-image) I'm guessing you are referring to the list above? Is there anything else? https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/stand... includes/meta/standard.tmpl:24: # : Primary Keeyword - Secondary Keyword | Brand Name On 2017/06/21 11:39:29, ire wrote: > Typo in "Keeyword" Done.
https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:29: # On 2017/06/21 12:05:01, juliandoucette wrote: > On 2017/06/21 11:39:29, ire wrote: > > NIT: Can you group the `og` and the `twitter` metadata together? So when we > read > > it we're not going back and forth between different types of metadata > > Acknowledged. > > I grouped them categorically. e.g. twitter:site after og:site, twitter:image:alt > after og:image. > > I'll take your suggestion. Done.
CC Vasily & Jon. Please find question below. https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/socia... includes/meta/social.tmpl:47: <meta property="og:url" content="{{ host }}/{{ page }}"> @Vasily and/or @Jon I need `protocol://host.domain:port/path`. How do you think I should do this? I would suggest a CMS global function that relies on protocol/host/domain/port configuration. -- Related to #5333.
On 2017/06/21 12:30:02, juliandoucette wrote: > @Vasily and/or @Jon I need `protocol://host.domain:port/path`. How do you think > I should do this? I would suggest a CMS global function that relies on > protocol/host/domain/port configuration. > > -- Related to #5333. So you're basically proposing to add a global function that returns the URL of current page, right? Something that would take the base URL of the website from the config and then add the path to the current page to it. Seems reasonable, especially if we make it a bit more generic and make it return the URL of any page. Something like `get_page_url(page=None, locale=None)` that by default would return the URL of the page that is being generated. Most of this functionality already exists in CMS (in `cms/sources.py:Source.resolve_link`) so it should not be too hard to add a wrapper to expose it as a global function. Would it be ok if I ask you to write a ticket and Jon and I take it from there?
https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:1: {# On 2017/06/21 12:05:01, juliandoucette wrote: > > Suggestion: Should we add the `fb:app_id`? This allows Facebook to tie our > > Facebook Page to shares of the page. It's mainly useful for additional > > analytics/insights so not sure if it's a priority > > Does fb:app_id apply to pages? Sorry I think I was unclear with my use of "page". When we add the fb:app_id to a site, whenever someone shares any url from that site, Facebook know's it is related to the Facebook Page App ID. And when we log into our own dashboard, we can see shares of the url. https://codereview.adblockplus.org/29469558/diff/29469579/includes/meta/socia... includes/meta/social.tmpl:68: {% endif %} > Falls back to og:image Noted. > Ack. I intend to create a separate include for articles/blog posts where things > like twitter:creator and og:article:author will be relevant. I don't think that > a twitter:creator is needed on other pages. Do you agree? Yeah I agree. > I'm guessing you are referring to the list above? Is there anything else? Yes I was referring to the list above. No there isn't anything else I could find.
https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/socia... includes/meta/social.tmpl:47: <meta property="og:url" content="{{ host }}/{{ page }}"> > So you're basically proposing to add a global function that returns the URL of > current page, right? Something that would take the base URL of the website from > the config and then add the path to the current page to it. Seems reasonable, > especially if we make it a bit more generic and make it return the URL of any > page. Something like `get_page_url(page=None, locale=None)` that by default > would return the URL of the page that is being generated. Most of this > functionality already exists in CMS (in `cms/sources.py:Source.resolve_link`) so > it should not be too hard to add a wrapper to expose it as a global function. > > Would it be ok if I ask you to write a ticket and Jon and I take it from there? Done. https://issues.adblockplus.org/ticket/5343
CC: Matze & Ferris (please tell me if I should just CC one or neither of you - or allow me to CC dev-ops via operations@eyeo.com in review) Please find question below https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... includes/meta/standard.tmpl:61: {% endif %} Looks like we're missing alternate links! :O Are we handling this already via HTTP headers? See: - https://support.google.com/webmasters/answer/189077 - https://webmasters.googleblog.com/2011/12/new-markup-for-multilingual-content...
- Added response from matze - Removed Ferris, Matze, Vasily, Jon (to avoid spamming them) https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... includes/meta/standard.tmpl:61: {% endif %} > CC: Matze & Ferris (please tell me if I should just CC one or neither of you - or allow me to CC dev-ops via operations@eyeo.com in review In Rietveld only individual people make sense. But don't hesitate to include anyone you consider suitable or necessary or potentially useful or similar > Are we handling this already via HTTP headers? No.
https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... includes/meta/standard.tmpl:61: {% endif %} Should the html5shiv and other IE-related scripts be included here as well?
Thanks Ire, please find your answer below. (@Lisa, @wspee @Thomas feel free to un-CC yourself to this code review if you don't want to follow. There will be a few patch sets and back-and-forths yet before it lands.) https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... includes/meta/standard.tmpl:61: {% endif %} On 2017/07/04 09:03:00, ire wrote: > Should the html5shiv and other IE-related scripts be included here as well? - I think it's a best practice to group link and script tags at the bottom of the head (if not the bottom of the body)? - I think that poly/pony fills don't have to be part of the minimum meta data on all websites Therefore I think that they belong in a separate meta include. There is a lot of "think" in this opinion :D - what do you think?
https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... includes/meta/standard.tmpl:61: {% endif %} On 2017/07/04 10:05:27, juliandoucette wrote: > On 2017/07/04 09:03:00, ire wrote: > > Should the html5shiv and other IE-related scripts be included here as well? > > - I think it's a best practice to group link and script tags at the bottom of > the head (if not the bottom of the body)? > - I think that poly/pony fills don't have to be part of the minimum meta data on > all websites > > Therefore I think that they belong in a separate meta include. > > There is a lot of "think" in this opinion :D - what do you think? I think (:P) you're right. I didn't necessarily mean they needed to go in this file, I just meant as a website-defaults meta.
https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29469558/diff/29470671/includes/meta/stand... includes/meta/standard.tmpl:61: {% endif %} On 2017/07/04 13:27:50, ire wrote: > I think (:P) you're right. I didn't necessarily mean they needed to go in this > file, I just meant as a website-defaults meta. In that case: Yes. 
|
