Issue 29680647: Issue 5329 - Provide a template for social media meta data in website-defaults (Closed)
|
Created:
Jan. 26, 2018, 11:28 a.m. by ire Modified:
Feb. 1, 2018, 9:07 a.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionIssue 5329 - Provide a template for social media meta data in website-defaults
Patch Set 1 #
Total comments: 60
Patch Set 2 : Addressed comments #3 #Patch Set 3 : Rebased #
Total comments: 4
Patch Set 4 : Addressed NITs #
MessagesTotal messages: 6
Ready for review. I added comments to most things that I changed from your initial implementation https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:10: get_string('name', 'site') if has_string('name', 'site') else None), I changed this to use the same site name string as the standard metadata https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:12: og_title, Should `og_title` be translated as well (since title is)? Same for og_description https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:29: config.get('social', 'twitter') if config.has_option('social', 'twitter') else '@eyeo'), 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 https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md File pages/metadata.md (right): https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:1: title=Metadata I added this page because 1) I think it's easier to understand what goes into this particular feature when the variables are abstracted from the code and 2) I think we should try and document our features more in general https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:6: code { font-family: monospace; } This could/should be in website-defaults. If you agree I can add it separately but, since it's only being used in this page, I'm also fine with leaving this here
> Ready for review. I added comments to most things that I changed from your > initial implementation Thanks :) https://codereview.adblockplus.org/29680647/diff/29680648/filters/to_og_local... File filters/to_og_locale.py (right): https://codereview.adblockplus.org/29680647/diff/29680648/filters/to_og_local... filters/to_og_locale.py:33: return og_locales[locale] Please consider this future-proof implementation https://hg.adblockplus.org/web.adblockplus.org/file/tip/filters/to_og_locale.py https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:2: ('og:url', NIT: I'm really not digging this ('multi', line style()), Looks like a mess to me :/ https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:5: ('og:type', 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. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:10: get_string('name', 'site') if has_string('name', 'site') else None), On 2018/01/26 11:33:59, ire wrote: > I changed this to use the same site name string as the standard metadata Acknowledged. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:10: get_string('name', 'site') if has_string('name', 'site') else None), NIT: `condition and output` can replace `output if condition else none` https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:12: og_title, On 2018/01/26 11:33:59, ire wrote: > Should `og_title` be translated as well (since title is)? Same for > og_description Yes. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:29: config.get('social', 'twitter') if config.has_option('social', 'twitter') else '@eyeo'), On 2018/01/26 11:33:59, ire wrote: > 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 Acknowledged. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:29: config.get('social', 'twitter') if config.has_option('social', 'twitter') else '@eyeo'), NIT: Long line https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:33: ('twitter:image', 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. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:49: {% macro meta_tag(property, content, defaultContent=None) -%} NIT: We could use this macro elsewhere if you separate it (into a separate file). https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:55: {% for property, content, defaultContent in social_media_meta %} 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')), ] https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:61: {{ meta_tag(('og:locale:alternate', alternate_locale | to_og_locale)) }} 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.). https://codereview.adblockplus.org/29680647/diff/29680648/pages/index.html File pages/index.html (right): https://codereview.adblockplus.org/29680647/diff/29680648/pages/index.html#ne... pages/index.html:23: <li><a href="content-styleguide">Content Styleguide</a></li> Detail: I think it makes sense to separate these from "Components" if we are going to separate "Includes" (separately). "Components" was intended to be very general and encompass includes. Despite that, I'm not opposed to making "Components" into SCSS components or something more specific if it helps us organize things better. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md File pages/metadata.md (right): https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:1: title=Metadata On 2018/01/26 11:33:59, ire wrote: > I added this page because 1) I think it's easier to understand what goes into > this particular feature when the variables are abstracted from the code and 2) I > think we should try and document our features more in general Acknowledged. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:6: code { font-family: monospace; } On 2018/01/26 11:33:59, ire wrote: > This could/should be in website-defaults. If you agree I can add it separately > but, since it's only being used in this page, I'm also fine with leaving this > here NIT: Please add it separately and remove the head block from templates/minimal (in this patchset at least). https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:20: The following variables can be set on a per page basis: NIT: Perhaps we should mention where/how they can be set e.g. via page meta data or {% set %}. (I would tend to call these "properties of a page" because they are usually defined in page front matter. Although variables may be more technically correct.) https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:24: - `og_site_name` NIT: Should not be page specific. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:29: - `og_locale` NIT: Should not be set by the user/developer. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:30: - `facebook_id` NIT: Should not be page specific? https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:35: - `pinterest_id` NIT: Should not be page specific? https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:39: Additionally, the following global variables can be set for the entire site. This is a locale string not a global variable. The CMS has a concept of globals defined in python in the globals directory. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:43: In a `locales/[locale]/site.json` file: I think that it must be defined in the defaultlocale to work (not throw an error)? https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:56: In the `settings.ini` file: NIT: I think "In `/settings.ini`" gets the point across (and communicates where the file resides in the repository relative to it's base). https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:60: facebook_id = 1234 NIT: Are these IDs actually short integers? https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:62: twitter = @username (Why do they make you include the @ sign!? :'( )
Thanks Julian! New patch set uploaded https://codereview.adblockplus.org/29680647/diff/29680648/filters/to_og_local... File filters/to_og_locale.py (right): https://codereview.adblockplus.org/29680647/diff/29680648/filters/to_og_local... filters/to_og_locale.py:33: return og_locales[locale] On 2018/01/30 04:04:39, juliandoucette wrote: > Please consider this future-proof implementation > https://hg.adblockplus.org/web.adblockplus.org/file/tip/filters/to_og_locale.py Done. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:2: ('og:url', On 2018/01/30 04:04:40, juliandoucette wrote: > NIT: I'm really not digging this > ('multi', > line > style()), > > Looks like a mess to me :/ 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 https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:5: ('og:type', On 2018/01/30 04:04:40, juliandoucette wrote: > 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. I doesn't seem that this data is implied without being specified. See doc - http://ogp.me/#types https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:10: get_string('name', 'site') if has_string('name', 'site') else None), On 2018/01/30 04:04:39, juliandoucette wrote: > NIT: `condition and output` can replace `output if condition else none` Are you saying a valid way to write this is `has_string('name', 'site') and get_string('name', 'site')` ? https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:12: og_title, On 2018/01/30 04:04:40, juliandoucette wrote: > On 2018/01/26 11:33:59, ire wrote: > > Should `og_title` be translated as well (since title is)? Same for > > og_description > > Yes. Done. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:29: config.get('social', 'twitter') if config.has_option('social', 'twitter') else '@eyeo'), On 2018/01/30 04:04:40, juliandoucette wrote: > NIT: Long line I can't think of any better way to write this than would be shorter :/ https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:33: ('twitter:image', On 2018/01/30 04:04:40, juliandoucette wrote: > 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. Ack. But there's no reason to exclude it now, is there? https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:49: {% macro meta_tag(property, content, defaultContent=None) -%} On 2018/01/30 04:04:40, juliandoucette wrote: > NIT: We could use this macro elsewhere if you separate it (into a separate > file). Done. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:55: {% for property, content, defaultContent in social_media_meta %} On 2018/01/30 04:04:40, juliandoucette wrote: > 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')), > ] 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. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:61: {{ meta_tag(('og:locale:alternate', alternate_locale | to_og_locale)) }} On 2018/01/30 04:04:40, juliandoucette wrote: > 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.). 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 https://codereview.adblockplus.org/29680647/diff/29680648/pages/index.html File pages/index.html (right): https://codereview.adblockplus.org/29680647/diff/29680648/pages/index.html#ne... pages/index.html:23: <li><a href="content-styleguide">Content Styleguide</a></li> On 2018/01/30 04:04:41, juliandoucette wrote: > Detail: I think it makes sense to separate these from "Components" if we are > going to separate "Includes" (separately). "Components" was intended to be very > general and encompass includes. Despite that, I'm not opposed to making > "Components" into SCSS components or something more specific if it helps us > organize things better. This makes sense. I think we should separate/organise these in a separate issue, could you create one please? https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md File pages/metadata.md (right): https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:6: code { font-family: monospace; } On 2018/01/30 04:04:41, juliandoucette wrote: > On 2018/01/26 11:33:59, ire wrote: > > This could/should be in website-defaults. If you agree I can add it separately > > but, since it's only being used in this page, I'm also fine with leaving this > > here > > NIT: Please add it separately and remove the head block from templates/minimal > (in this patchset at least). https://codereview.adblockplus.org/29684561 https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:20: The following variables can be set on a per page basis: On 2018/01/30 04:04:41, juliandoucette wrote: > NIT: Perhaps we should mention where/how they can be set e.g. via page meta data > or {% set %}. I think this is more global "how to use the cms" information, rather than specific to this include. > (I would tend to call these "properties of a page" because they are usually > defined in page front matter. Although variables may be more technically > correct.) Changed. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:24: - `og_site_name` On 2018/01/30 04:04:41, juliandoucette wrote: > NIT: Should not be page specific. I think it's very unlikely, but perhaps it could be different for different pages. I think the option should be there just in case. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:29: - `og_locale` On 2018/01/30 04:04:41, juliandoucette wrote: > NIT: Should not be set by the user/developer. Same comment as above. I can't really imagine a case where we would want to set it, but does it hurt to have the option? Particularly as there is a default that will likely be used in most cases. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:30: - `facebook_id` On 2018/01/30 04:04:41, juliandoucette wrote: > NIT: Should not be page specific? I can imagine that there could be different pages for different products within the same site that would have different facebook_id (and pinterest_id) https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:35: - `pinterest_id` On 2018/01/30 04:04:41, juliandoucette wrote: > NIT: Should not be page specific? See comment above. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:39: Additionally, the following global variables can be set for the entire site. On 2018/01/30 04:04:41, juliandoucette wrote: > This is a locale string not a global variable. The CMS has a concept of globals > defined in python in the globals directory. I didn't mean it as a literal global variable. I meant it more that this is a property/variable/whatever that is available globally. I've changed the wording though, let me know if it works. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:43: In a `locales/[locale]/site.json` file: On 2018/01/30 04:04:41, juliandoucette wrote: > I think that it must be defined in the defaultlocale to work (not throw an > error)? I don't think so. There's no file in website-defaults and there is no error thrown. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:56: In the `settings.ini` file: On 2018/01/30 04:04:41, juliandoucette wrote: > NIT: I think "In `/settings.ini`" gets the point across (and communicates where > the file resides in the repository relative to it's base). Done. https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:60: facebook_id = 1234 On 2018/01/30 04:04:41, juliandoucette wrote: > NIT: Are these IDs actually short integers? I don't think so, I think they may be alphanumeric strings https://codereview.adblockplus.org/29680647/diff/29680648/pages/metadata.md#n... pages/metadata.md:62: twitter = @username On 2018/01/30 04:04:41, juliandoucette wrote: > (Why do they make you include the @ sign!? :'( ) Done.
LGTM + NITs https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:2: ('og:url', On 2018/01/30 08:19:08, ire wrote: > On 2018/01/30 04:04:40, juliandoucette wrote: > > NIT: I'm really not digging this > > ('multi', > > line > > style()), > > > > Looks like a mess to me :/ > > 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 Acknowledged. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:5: ('og:type', On 2018/01/30 08:19:08, ire wrote: > On 2018/01/30 04:04:40, juliandoucette wrote: > > 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. > > I doesn't seem that this data is implied without being specified. See doc - > http://ogp.me/#types > Acknowledged. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:10: get_string('name', 'site') if has_string('name', 'site') else None), On 2018/01/30 08:19:08, ire wrote: > On 2018/01/30 04:04:39, juliandoucette wrote: > > NIT: `condition and output` can replace `output if condition else none` > > Are you saying a valid way to write this is `has_string('name', 'site') and > get_string('name', 'site')` ? Yes. But apparently you can also just drop the `else None`. This seems to working Jinja and not in pure python. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:29: config.get('social', 'twitter') if config.has_option('social', 'twitter') else '@eyeo'), On 2018/01/30 08:19:08, ire wrote: > On 2018/01/30 04:04:40, juliandoucette wrote: > > NIT: Long line > > I can't think of any better way to write this than would be shorter :/ Acknowledged. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:33: ('twitter:image', On 2018/01/30 08:19:08, ire wrote: > On 2018/01/30 04:04:40, juliandoucette wrote: > > 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. > > Ack. But there's no reason to exclude it now, is there? We should not maintain code that we do not and probably will not use. https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:55: {% for property, content, defaultContent in social_media_meta %} On 2018/01/30 08:19:08, ire wrote: > On 2018/01/30 04:04:40, juliandoucette wrote: > > 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')), > > ] > > 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. Acknowledged. https://codereview.adblockplus.org/29680647/diff/29685566/filters/to_og_local... File filters/to_og_locale.py (right): https://codereview.adblockplus.org/29680647/diff/29685566/filters/to_og_local... filters/to_og_locale.py:33: NIT: Extra whitespace https://codereview.adblockplus.org/29680647/diff/29685566/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29685566/includes/meta/socia... includes/meta/social.tmpl:5: og_url, NIT: Extra whitespace at the end of every line? :/
https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:10: get_string('name', 'site') if has_string('name', 'site') else None), On 2018/01/31 19:03:21, juliandoucette wrote: > On 2018/01/30 08:19:08, ire wrote: > > On 2018/01/30 04:04:39, juliandoucette wrote: > > > NIT: `condition and output` can replace `output if condition else none` > > > > Are you saying a valid way to write this is `has_string('name', 'site') and > > get_string('name', 'site')` ? > > Yes. But apparently you can also just drop the `else None`. This seems to > working Jinja and not in pure python. The line `has_string('name', 'site') and get_string('name', 'site')` throws an error I dropped the `else None` https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/socia... includes/meta/social.tmpl:33: ('twitter:image', On 2018/01/31 19:03:21, juliandoucette wrote: > On 2018/01/30 08:19:08, ire wrote: > > On 2018/01/30 04:04:40, juliandoucette wrote: > > > 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. > > > > Ack. But there's no reason to exclude it now, is there? > > We should not maintain code that we do not and probably will not use. It will be used for pages that already have a featured image, since that is the fallback. e.g. the acceptableads.com blog posts. https://codereview.adblockplus.org/29680647/diff/29685566/filters/to_og_local... File filters/to_og_locale.py (right): https://codereview.adblockplus.org/29680647/diff/29685566/filters/to_og_local... filters/to_og_locale.py:33: On 2018/01/31 19:03:22, juliandoucette wrote: > NIT: Extra whitespace Done. https://codereview.adblockplus.org/29680647/diff/29685566/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29685566/includes/meta/socia... includes/meta/social.tmpl:5: og_url, On 2018/01/31 19:03:22, juliandoucette wrote: > NIT: Extra whitespace at the end of every line? :/ Done. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
