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

Issue 29480630: Issue 5329 - Added standard meta data to website-defaults (Closed)

Created:
July 5, 2017, 5:34 p.m. by juliandoucette
Modified:
July 18, 2017, 9:57 a.m.
Reviewers:
ire, saroyanm
Base URL:
https://hg.adblockplus.org/website-defaults
Visibility:
Public.

Description

Issue 5329 - Added standard meta data to website-defaults

Patch Set 1 #

Total comments: 6

Patch Set 2 : Removed title condition #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -0 lines) Patch
A includes/meta/standard.tmpl View 1 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 8
juliandoucette
July 5, 2017, 5:34 p.m. (2017-07-05 17:34:52 UTC) #1
juliandoucette
I decided to break up this issue into several reviews for the sake of efficiency. ...
July 5, 2017, 5:36 p.m. (2017-07-05 17:36:20 UTC) #2
ire
Thanks, this is much better broken up. https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl#newcode4 includes/meta/standard.tmpl:4: {% if ...
July 6, 2017, 8:12 a.m. (2017-07-06 08:12:02 UTC) #3
juliandoucette
https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl#newcode4 includes/meta/standard.tmpl:4: {% if title %} On 2017/07/06 08:12:02, ire wrote: ...
July 6, 2017, 1:26 p.m. (2017-07-06 13:26:00 UTC) #4
ire
LGTM See my suggestion below. https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl#newcode4 includes/meta/standard.tmpl:4: {% if title %} ...
July 7, 2017, 8:37 a.m. (2017-07-07 08:37:44 UTC) #5
juliandoucette
https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl#newcode4 includes/meta/standard.tmpl:4: {% if title %} On 2017/07/07 08:37:43, ire wrote: ...
July 7, 2017, 11:50 a.m. (2017-07-07 11:50:17 UTC) #6
ire
Sorry for the delay. Answer below. https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl File includes/meta/standard.tmpl (right): https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/standard.tmpl#newcode4 includes/meta/standard.tmpl:4: {% if title ...
July 12, 2017, 8:22 a.m. (2017-07-12 08:22:38 UTC) #7
juliandoucette
July 14, 2017, 11:40 a.m. (2017-07-14 11:40:54 UTC) #8
https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/stand...
File includes/meta/standard.tmpl (right):

https://codereview.adblockplus.org/29480630/diff/29480631/includes/meta/stand...
includes/meta/standard.tmpl:4: {% if title %}
On 2017/07/12 08:22:37, ire wrote:
> On 2017/07/07 11:50:16, juliandoucette wrote:
> > On 2017/07/07 08:37:43, ire wrote:
> > > This is probably a suggestion that can be dealt with in a separate issue,
> but
> > do
> > > you think it would make sense to have a site-wide fallback then? 
> > 
> > I don't know. 
> > 
> > In (my) theory - a competent parser (search engine, social media) will fall
> back
> > to the highest level heading (title) and the first phrasing content it can
> find
> > (description) and this may be better than the site title/description because
> we
> > don't want people going to specific (non-home) pages that look like home in
> > search results / social media. - But I'm really speculating here - and I'm
> > guessing I'm wrong about something.
> > 
> > Do you know anything about this?
> 
> From my (limited) research, I couldn't really find anything to support this
> fallback behaviour. Additionally, according to the HTML spec, a title element
is
> required in most circumstances
> (http://w3c.github.io/html/single-page.html#the-head-element) and a page will
> fail validation if it is not included. 
> 
> I don't think this is the same for description though. So perhaps a fallback
> would be necessary for the page title, but not the description. 

Acknowledged.

I was wrong about Facebook; at least. See
https://developers.facebook.com/tools/debug/sharing/?q=http%3A%2F%2Fjuliandou...

Powered by Google App Engine
This is Rietveld