|
|
Created:
July 5, 2017, 9:43 p.m. by juliandoucette Modified:
Sept. 25, 2017, 2:35 p.m. Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionIssue 5329 - Added standard social media meta data to website-defaults
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixed typo on og:description #Patch Set 3 : Added facebook and printrist ids #
Total comments: 1
Patch Set 4 : Fixed "pinterest" typo #
Total comments: 9
Patch Set 5 : Added og(title|description) fallbacks #Patch Set 6 : Added license header #Patch Set 7 : Refactored defaults and updated locale logic #
Total comments: 2
Patch Set 8 : Renamed filter and replaced double quotes #Patch Set 9 : Added missing translate filters #
Total comments: 1
Patch Set 10 : Removed test page in favour of default template #Patch Set 11 : Refactored output, added og:image:alt #Patch Set 12 : Rebased #
MessagesTotal messages: 30
Note: This includes standard website meta data for social media. There will be another include that contains article specific meta data. https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:4: <meta property="og:url" content="{{ host }}/{{ page }}"> Note: I created an issue to provide us with a better way to do this (instead of using a global `host`) https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:23: <meta property="og:locale" content="{{ locale }}"> Note: I'm not sure if the accepted locales are the same as the locales we provide.
On 2017/07/05 21:47:25, juliandoucette wrote: > Note: This includes standard website meta data for social media. There will be > another include that contains article specific meta data. Another include - in another commit/review*
Thanks. A couple general comments: 1. In the other review you mentioned something about a field for pinterest. Is that still happening? 2. Did we conclude on whether or not to include fb:app_id ? https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:4: <meta property="og:url" content="{{ host }}/{{ page }}"> On 2017/07/05 21:47:25, juliandoucette wrote: > Note: I created an issue to provide us with a better way to do this (instead of > using a global `host`) Acknowledged. https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:16: <meta property="og:descirption" content="{{ og_description }}"> Typo in "og:descirption" https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:23: <meta property="og:locale" content="{{ locale }}"> Should this be wrapped in an if statement? Or is locale a required variable?
> 1. In the other review you mentioned something about a field forpinterest. Is that still happening? Yes. > 2. Did we conclude on whether or not to include fb:app_id ? No. I haven't decided where I want to draw the line between standard and proprietary here. Unfortunately I was forced to cross the line already for twitter - so it's a little hard to argue against facebook and pintrist. https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:16: <meta property="og:descirption" content="{{ og_description }}"> On 2017/07/07 16:05:06, ire wrote: > Typo in "og:descirption" Done. Good eye. https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:23: <meta property="og:locale" content="{{ locale }}"> On 2017/07/07 16:05:06, ire wrote: > Should this be wrapped in an if statement? Or is locale a required variable? locale is always set. A default must be set in settings.ini.
On 2017/07/07 20:05:20, juliandoucette wrote: > I haven't decided where I want to draw the line between standard and proprietary > here. Unfortunately I was forced to cross the line already for twitter - so it's > a little hard to argue against facebook and pintrist. I've added them in the latest patchset.
https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:16: <meta property="og:descirption" content="{{ og_description }}"> On 2017/07/07 20:05:20, juliandoucette wrote: > On 2017/07/07 16:05:06, ire wrote: > > Typo in "og:descirption" > > Done. > > Good eye. Thanks :) https://codereview.adblockplus.org/29480675/diff/29480676/includes/meta/socia... includes/meta/social.tmpl:23: <meta property="og:locale" content="{{ locale }}"> On 2017/07/07 20:05:20, juliandoucette wrote: > On 2017/07/07 16:05:06, ire wrote: > > Should this be wrapped in an if statement? Or is locale a required variable? > > locale is always set. A default must be set in settings.ini. Acknowledged. https://codereview.adblockplus.org/29480675/diff/29482730/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29482730/includes/meta/socia... includes/meta/social.tmpl:52: {# pintrist #} It's "pinterest" ;)
Notes below. https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:13: <meta property="og:title" content="{{ og_title }}"> I think I should add a fallback here for social networks that may not fall back to title. https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:16: <meta property="og:description" content="{{ og_description }}"> I think I should add a fallback here for social networks that may not fall back to description.
https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:13: <meta property="og:title" content="{{ og_title }}"> On 2017/07/11 12:10:45, juliandoucette wrote: > I think I should add a fallback here for social networks that may not fall back > to title. I agree. I may be confused with your term "fallback" here, but isn't this tag supposed to be the fallback, and the proprietary tags the "enhancement"?
likewise for og:description https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:13: <meta property="og:title" content="{{ og_title }}"> On 2017/07/12 07:55:19, ire wrote: > I may be confused with your term "fallback" here, but isn't this tag supposed to > be the fallback, and the proprietary tags the "enhancement"? I'm suggesting: if og_title og:title = og_title else if title og:title = title both title and og_title may be set in the frontmatter of pages.
https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:13: <meta property="og:title" content="{{ og_title }}"> On 2017/07/12 15:52:38, juliandoucette wrote: > On 2017/07/12 07:55:19, ire wrote: > > I may be confused with your term "fallback" here, but isn't this tag supposed > to > > be the fallback, and the proprietary tags the "enhancement"? > > I'm suggesting: > > if og_title > og:title = og_title > else if title > og:title = title > > both title and og_title may be set in the frontmatter of pages. Ah right. I read your initial statement in the wrong way. What you are suggesting looks good. A thought: are there ever cases where `og_title` would be different from `title`? If not we may only actually need title. At the same time it could be useful to have the option to set something different. What do you think?
https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:13: <meta property="og:title" content="{{ og_title }}"> On 2017/07/13 09:39:46, ire wrote: > A thought: are there ever cases where `og_title` would be different from > `title`? If not we may only actually need title. At the same time it could be > useful to have the option to set something different. What do you think? RE: Will title be different than og:title (and alike) Probably not. But they should be. Optimizing for search engines is different than optimizing for social media. Search title : `PRIMARY_KEYWORD SECONDARY_KEYWORD | BRAND_NAME` 60 chars (https://moz.com/learn/seo/title-tag) Social title : `PAGE_HEADING` 40 chars, "A clear title without branding or mentioning the domain itself." (https://developers.facebook.com/docs/sharing/best-practices) https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:13: <meta property="og:title" content="{{ og_title }}"> On 2017/07/11 12:10:45, juliandoucette wrote: > I think I should add a fallback here for social networks that may not fall back > to title. Done. https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:16: <meta property="og:description" content="{{ og_description }}"> On 2017/07/11 12:10:45, juliandoucette wrote: > I think I should add a fallback here for social networks that may not fall back > to description. Done.
LGTM https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29480675/diff/29485607/includes/meta/socia... includes/meta/social.tmpl:13: <meta property="og:title" content="{{ og_title }}"> On 2017/07/14 11:27:31, juliandoucette wrote: > On 2017/07/13 09:39:46, ire wrote: > > A thought: are there ever cases where `og_title` would be different from > > `title`? If not we may only actually need title. At the same time it could be > > useful to have the option to set something different. What do you think? > > RE: Will title be different than og:title (and alike) > > Probably not. But they should be. > > Optimizing for search engines is different than optimizing for social media. > > Search title > : `PRIMARY_KEYWORD SECONDARY_KEYWORD | BRAND_NAME` 60 chars > (https://moz.com/learn/seo/title-tag) > > Social title > : `PAGE_HEADING` 40 chars, "A clear title without branding or mentioning the > domain itself." (https://developers.facebook.com/docs/sharing/best-practices) Okay makes sense. Thanks.
+Vasily +Jon Can you please review my tooglocale filter? Ideally, I would load https://api.crowdin.com/api/supported-languages and match `crowdin_code` to `locale` and then change "-" to "_" and apply the exceptions documented here https://developers.facebook.com/docs/internationalization. Instead (because [I didn't think we should query this API every build, I didn't want to write the code to query this API, I didn't know how to load an xml file]) I performed this matching manually for the locales I found in web.adblockplus.org's settings.ini (including the incorrect one that I think I found... see https://issues.adblockplus.org/ticket/5479) Note: I would normally separate words by "_" in python but I looked at the built-in jinja2 filters (http://jinja.pocoo.org/docs/2.9/templates/#builtin-filters) and they seem to be non-separated.
(I would like to prioritize this because I could fix https://issues.adblockplus.org/ticket/1274 and https://issues.adblockplus.org/ticket/1198 using this filter)
Hi Julian, The filter looks good, except for the nit about the quotes (see below). Your reasoning about the naming convention for filters makes sense, although I would probably add the underscores anyway, since `tooglocale` is just too hard to parse without. Anyway, I'd be happy with whatever naming you prefer. Cheers, Vasily https://codereview.adblockplus.org/29480675/diff/29509679/filters/tooglocale.py File filters/tooglocale.py (right): https://codereview.adblockplus.org/29480675/diff/29509679/filters/tooglocale.... filters/tooglocale.py:2: "ar": "ar_AR", If our Python coding style applies to the websites (this is kind of your call, I guess, but I'd advise for it) it's preferable to use single quotes on short strings that don't contain single quotes inside.
On 2017/08/09 17:02:48, Vasily Kuznetsov wrote: > Hi Julian, > > The filter looks good, except for the nit about the quotes (see below). Your > reasoning about the naming convention for filters makes sense, although I would > probably add the underscores anyway, since `tooglocale` is just too hard to > parse without. Anyway, I'd be happy with whatever naming you prefer. > > Cheers, > Vasily > > https://codereview.adblockplus.org/29480675/diff/29509679/filters/tooglocale.py > File filters/tooglocale.py (right): > > https://codereview.adblockplus.org/29480675/diff/29509679/filters/tooglocale.... > filters/tooglocale.py:2: "ar": "ar_AR", > If our Python coding style applies to the websites (this is kind of your call, I > guess, but I'd advise for it) it's preferable to use single quotes on short > strings that don't contain single quotes inside. Hi Julian, I agree with Vasily as well :)
I renamed the filter and replaced the double quotes with single quotes. I'd like to generate and load this locale list instead of hard coding it in the future. But I'm ok with hard coding it for now in order to resolve the issues that I mentioned. Will you please review my latest changes ire and/or saroyanm? https://codereview.adblockplus.org/29480675/diff/29509679/filters/tooglocale.py File filters/tooglocale.py (right): https://codereview.adblockplus.org/29480675/diff/29509679/filters/tooglocale.... filters/tooglocale.py:2: "ar": "ar_AR", On 2017/08/09 17:02:48, Vasily Kuznetsov wrote: > If our Python coding style applies to the websites (this is kind of your call, I > guess, but I'd advise for it) it's preferable to use single quotes on short > strings that don't contain single quotes inside. Done.
(I removed kvas and jsonesen from CC to avoid spamming them) Added missing translation filters and a dummy test page. I haven't figured out how I want to document this component (and others like it) yet. The first thing that comes to mind is to create a template that implements these includes and a couple pages that demonstrate their configurations (e.g. min vs max configuration). https://codereview.adblockplus.org/29480675/diff/29510640/pages/test.md File pages/test.md (right): https://codereview.adblockplus.org/29480675/diff/29510640/pages/test.md#newcode1 pages/test.md:1: title=test Note: This is a dummy page that demonstrates the easiest way to test this include. It is not meant for production.
LGTM for filters/to_og_locale.py in PS9.
@kvas Updated Patchset is mostly Jinja :) I don't know how to localize meta images. In content, the CMS automatically corrects relative URLs. But the same is not true for meta tags. And meta tags require absolute URLs. Do you have any ideas?
On 2017/08/21 12:01:15, juliandoucette wrote: > @kvas > > Updated Patchset is mostly Jinja :) > > I don't know how to localize meta images. In content, the CMS automatically > corrects relative URLs. But the same is not true for meta tags. And meta tags > require absolute URLs. Do you have any ideas? Hi Julian, This is a bit too abstract for me, can you give me a small example of how an image URL gets autocorrected in the content but ends up wrong in the meta tag? Cheers, Vasily
On 2017/08/21 17:26:21, Vasily Kuznetsov wrote: > Hi Julian, > > This is a bit too abstract for me, can you give me a small example of how an > image URL gets autocorrected in the content but ends up wrong in the meta tag? > > Cheers, > Vasily Sorry Vasily. See process_links in converters.py (https://hg.adblockplus.org/cms/file/tip/cms/converters.py#l266) The CMS automatically localizes a.hrefs and img.srcs but it doesn't handle meta.content. meta.content image paths must be full paths not relative paths. I guess I could use siteurl and locale to generate a full URL that will resolve correctly. But it seems we should handle this consistently e.g. via global function. (Images may or may not be localized. The default language image goes in the static folder. Other language images go inside the locales folder. generate_static_pages moves these images appropriately and our nginx config will fall back to the default language image when the locale image cannot be found.)
On 2017/08/21 19:15:24, juliandoucette wrote: > On 2017/08/21 17:26:21, Vasily Kuznetsov wrote: > > Hi Julian, > > > > This is a bit too abstract for me, can you give me a small example of how an > > image URL gets autocorrected in the content but ends up wrong in the meta tag? > > > > Cheers, > > Vasily > > Sorry Vasily. > > See process_links in converters.py > (https://hg.adblockplus.org/cms/file/tip/cms/converters.py#l266) > > The CMS automatically localizes a.hrefs and img.srcs but it doesn't handle > meta.content. > > meta.content image paths must be full paths not relative paths. > > I guess I could use siteurl and locale to generate a full URL that will resolve > correctly. But it seems we should handle this consistently e.g. via global > function. > > (Images may or may not be localized. The default language image goes in the > static folder. Other language images go inside the locales folder. > generate_static_pages moves these images appropriately and our nginx config will > fall back to the default language image when the locale image cannot be found.) I looked at CMS code and at the images and animations in web.adblockplus.org repository and here's what I see: - The CMS can localize links to images if the default image is placed into 'locales/en/images/foo.png' and language-specific versions are placed in 'locales/de/images/foo.png' (here 'de' can be any locale, 'images' can be replaced with any folder that's used for storing images and 'foo.png' can be any image). - As far as I see from CMS code, if the image is not present in 'locales/en' (or whatever the default locale is, it won't be localized and a broken link warning will be printed). - There's a custom filter used for inlining images in other tags (e.g. <object>), it's called 'inline_file' and it also supports localization. The standard link localtization behavior is taken care of by `sources.py:Source.resolve_link`. It's accessible to the filters as `context['source'].resolve_link
On 2017/08/22 10:43:02, Vasily Kuznetsov wrote: > On 2017/08/21 19:15:24, juliandoucette wrote: > > On 2017/08/21 17:26:21, Vasily Kuznetsov wrote: > > > Hi Julian, > > > > > > This is a bit too abstract for me, can you give me a small example of how an > > > image URL gets autocorrected in the content but ends up wrong in the meta > tag? > > > > > > Cheers, > > > Vasily > > > > Sorry Vasily. > > > > See process_links in converters.py > > (https://hg.adblockplus.org/cms/file/tip/cms/converters.py#l266) > > > > The CMS automatically localizes a.hrefs and img.srcs but it doesn't handle > > meta.content. > > > > meta.content image paths must be full paths not relative paths. > > > > I guess I could use siteurl and locale to generate a full URL that will > resolve > > correctly. But it seems we should handle this consistently e.g. via global > > function. > > > > (Images may or may not be localized. The default language image goes in the > > static folder. Other language images go inside the locales folder. > > generate_static_pages moves these images appropriately and our nginx config > will > > fall back to the default language image when the locale image cannot be > found.) > > I looked at CMS code and at the images and animations in http://web.adblockplus.org > repository and here's what I see: > > - The CMS can localize links to images if the default image is placed into > 'locales/en/images/foo.png' and language-specific versions are placed in > 'locales/de/images/foo.png' (here 'de' can be any locale, 'images' can be > replaced with any folder that's used for storing images and 'foo.png' can be any > image). > - As far as I see from CMS code, if the image is not present in 'locales/en' (or > whatever the default locale is, it won't be localized and a broken link warning > will be printed). > - There's a custom filter used for inlining images in other tags (e.g. > <object>), it's called 'inline_file' and it also supports localization. > > The standard link localtization behavior is taken care of by > `sources.py:Source.resolve_link`. It's accessible to the filters as > `context['source'].resolve_link Sorry, posted an unfinished comment by accident. So anyway, `sources.py:Source.resolve_link` is accessible to the filters as `context['source'].resolve_link` and this can be used in a custom filter now, without any changes to the CMS. We can also add this filter (or global function, if that's better) to the CMS, if that's helpful. You can create a ticket for that.
On 2017/08/22 10:47:27, Vasily Kuznetsov wrote: > So anyway, `sources.py:Source.resolve_link` is accessible to the filters as > `context['source'].resolve_link` and this can be used in a custom filter now, > without any changes to the CMS. We can also add this filter (or global function, > if that's better) to the CMS, if that's helpful. You can create a ticket for > that. @Vasily It depends... Would you (personally) rather resolve this link via global function or filter or via converter like the other images on the page? I think it would be most consistent to add another regex to the code that I linked for meta.content when meta.content contains a link to an image. But I get the sense that these converter features are something we want to move out of cms and into website-defaults or move away from entirely. What do you think?
On 2017/08/22 11:15:35, juliandoucette wrote: > On 2017/08/22 10:47:27, Vasily Kuznetsov wrote: > > > So anyway, `sources.py:Source.resolve_link` is accessible to the filters as > > `context['source'].resolve_link` and this can be used in a custom filter now, > > without any changes to the CMS. We can also add this filter (or global > function, > > if that's better) to the CMS, if that's helpful. You can create a ticket for > > that. > > @Vasily > > It depends... > > Would you (personally) rather resolve this link via global function or filter or > via converter like the other images on the page? > > I think it would be most consistent to add another regex to the code that I > linked for meta.content when meta.content contains a link to an image. But I get > the sense that these converter features are something we want to move out of cms > and into website-defaults or move away from entirely. > > What do you think? I haven't thought about augmenting the converters, but it's a good option. It's rather trivial to add support for <meta content="..."> and not very difficult to enable more flexible configurability (for example we could enable adding regexps via settings.ini).
On 2017/08/22 14:30:00, Vasily Kuznetsov wrote: > I haven't thought about augmenting the converters, but it's a good option. It's > rather trivial to add support for <meta content="..."> and not very difficult to > enable more flexible configurability (for example we could enable adding regexps > via settings.ini). I mostly care about finishing this review :D I will either do something like: <meta property="og:image" content="{{ source.resolve_link(og_image) }}"> or: <meta property="og:image" content="{{ og_image }}"> depending on whether you think it's best to add meta.content to converters.py I think regex config in settings.ini would be a little extreme and very ugly. I'd rather implement converters like these in website-defaults if given the ability like we discussed elsewhere. But I don't mind you adding meta.content support to converters.py in the meantime if you think that's a better solution than using source.resolve_link (assuming source.resolve_link works for these requirements -> localized image, full url).
On 2017/08/22 14:45:30, juliandoucette wrote: > On 2017/08/22 14:30:00, Vasily Kuznetsov wrote: > > I haven't thought about augmenting the converters, but it's a good option. > It's > > rather trivial to add support for <meta content="..."> and not very difficult > to > > enable more flexible configurability (for example we could enable adding > regexps > > via settings.ini). > > I mostly care about finishing this review :D > > I will either do something like: > > <meta property="og:image" content="{{ source.resolve_link(og_image) }}"> > > or: > > <meta property="og:image" content="{{ og_image }}"> > > depending on whether you think it's best to add meta.content to converters.py > > I think regex config in settings.ini would be a little extreme and very ugly. > I'd rather implement converters like these in website-defaults if given the > ability like we discussed elsewhere. But I don't mind you adding meta.content > support to converters.py in the meantime if you think that's a better solution > than using source.resolve_link (assuming source.resolve_link works for these > requirements -> localized image, full url). I'm happy to add meta.content to converters.py (https://issues.adblockplus.org/ticket/5557), but that will take a bit of time. Can this review wait for it? Otherwise you can land it with the first option and then update after the converters.py change lands.
On 2017/08/22 17:33:22, Vasily Kuznetsov wrote: > I'm happy to add meta.content to converters.py > (https://issues.adblockplus.org/ticket/5557), but that will take a bit of time. > Can this review wait for it? Otherwise you can land it with the first option and > then update after the converters.py change lands. I think it can wait. At least until we can actually share code from website-defaults to websites :D |