Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -0 lines) Patch
A filters/to_og_locale.py View 1 2 3 1 chunk +32 lines, -0 lines 0 comments Download
A includes/meta/social.tmpl View 1 2 3 1 chunk +53 lines, -0 lines 0 comments Download
A macros/meta-tag.tmpl View 1 1 chunk +5 lines, -0 lines 0 comments Download
M pages/index.html View 1 chunk +6 lines, -0 lines 0 comments Download
A pages/metadata.md View 1 2 1 chunk +58 lines, -0 lines 0 comments Download
M templates/minimal.tmpl View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 6
ire
Jan. 26, 2018, 11:28 a.m. (2018-01-26 11:28:09 UTC) #1
ire
Ready for review. I added comments to most things that I changed from your initial ...
Jan. 26, 2018, 11:34 a.m. (2018-01-26 11:34:00 UTC) #2
juliandoucette
> Ready for review. I added comments to most things that I changed from your ...
Jan. 30, 2018, 4:04 a.m. (2018-01-30 04:04:42 UTC) #3
ire
Thanks Julian! New patch set uploaded https://codereview.adblockplus.org/29680647/diff/29680648/filters/to_og_locale.py File filters/to_og_locale.py (right): https://codereview.adblockplus.org/29680647/diff/29680648/filters/to_og_locale.py#newcode33 filters/to_og_locale.py:33: return og_locales[locale] On ...
Jan. 30, 2018, 8:19 a.m. (2018-01-30 08:19:10 UTC) #4
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/social.tmpl File includes/meta/social.tmpl (right): https://codereview.adblockplus.org/29680647/diff/29680648/includes/meta/social.tmpl#newcode2 includes/meta/social.tmpl:2: ('og:url', On 2018/01/30 08:19:08, ire wrote: ...
Jan. 31, 2018, 7:03 p.m. (2018-01-31 19:03:22 UTC) #5
ire
Feb. 1, 2018, 9:07 a.m. (2018-02-01 09:07:02 UTC) #6
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.

Powered by Google App Engine
This is Rietveld