Issue 5442 - Add logo microdata to eyeo websites

Issue 5442 - Add logo microdata to eyeo websites

[ire, 2017-12-18 08:58:24]

Ready for review.

A couple of notes:

- I created a new size of logo because the image needs to be at least 112x112
(https://developers.google.com/search/docs/data-types/logo#type_definitions)
- I decided to put the microdata just on the homepage, because it seems that
different pages will have additional/different microdata. E.g. on the jobs pages
we could add the Job Posting Microdata
(https://developers.google.com/search/docs/data-types/job-posting). Also, based
on the guidelines for the location of microdata
(https://developers.google.com/search/docs/guides/sd-policies#location), it
seems like we shouldn't just put the same microdata on all pages, each page's
microdata should depend on the content.

[juliandoucette, 2017-12-18 13:21:59]

First impression.

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html
File pages/index.html (right):

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:48: <script type="application/ld+json">
See first comment in default.tmpl

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:51: "@type": "Organization",
TOL: We could define this in python (e.g. meta data, global, or include) and
then use a global/filter to output it in JSON

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:51: "@type": "Organization",
TOL: Now that we are defining Organization meta data, it seems like things are
obviously missing e.g. "name". This is ok, and we can add anything that we want
separately, as long as defining this script block doesn't override other forms
of macro data collection (as long as it's not one format or the other).

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.t...
templates/default.tmpl:104: <? include meta/microdata ?>
I think that we put scripts at the bottom of the body so that they don't delay
rendering in old (possibly no longer supported) browsers that don't support
defer/async. I don't think that this makes sense for inline json?

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.t...
templates/default.tmpl:104: <? include meta/microdata ?>
Also, it looks like you put this here by mistake because you didn't include a
meta/microdata include in this review :D

[ire, 2017-12-20 08:26:00]

New patch

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html
File pages/index.html (right):

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:48: <script type="application/ld+json">
On 2017/12/18 13:21:59, juliandoucette wrote:
> See first comment in default.tmpl

Acknowledged.

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:51: "@type": "Organization",
On 2017/12/18 13:21:59, juliandoucette wrote:
> TOL: Now that we are defining Organization meta data, it seems like things are
> obviously missing e.g. "name". This is ok, and we can add anything that we
want
> separately, as long as defining this script block doesn't override other forms
> of macro data collection (as long as it's not one format or the other).

- Yes there are already other issues for adding more microdata information
- I don't think it's "one format or the other", I think we can supplement the
markup microdata. But we can also just add the additional microdata here, this
JSON format is the preferred version according to the docs

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:51: "@type": "Organization",
On 2017/12/18 13:21:59, juliandoucette wrote:
> TOL: We could define this in python (e.g. meta data, global, or include) and
> then use a global/filter to output it in JSON

Sorry, I don't quite understand. Since the data will be different for each page,
what exactly should we define in a python global? Just this line that specifies
the @type?

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.t...
templates/default.tmpl:104: <? include meta/microdata ?>
On 2017/12/18 13:21:59, juliandoucette wrote:
> Also, it looks like you put this here by mistake because you didn't include a
> meta/microdata include in this review :D

Oops! Yes I previously had this file but since the metadata on each page will be
different, I decided to make it inline. Removing

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.t...
templates/default.tmpl:104: <? include meta/microdata ?>
On 2017/12/18 13:21:59, juliandoucette wrote:
> I think that we put scripts at the bottom of the body so that they don't delay
> rendering in old (possibly no longer supported) browsers that don't support
> defer/async. I don't think that this makes sense for inline json?

I agree that we don't *need* to, but is there a reason to not put it at the
bottom anyway? According to the docs
(https://developers.google.com/search/docs/guides/intro-structured-data) it just
says it can be placed in the <head> or <body> . I guess that this could be
considered metadata so perhaps we should put it in the <head>?

[juliandoucette, 2017-12-20 11:50:17]

Is there a reason that you chose to use the white logo instead of the black?

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html
File pages/index.html (right):

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:51: "@type": "Organization",
On 2017/12/20 08:25:59, ire wrote:
> Sorry, I don't quite understand. Since the data will be different for each
page,
> what exactly should we define in a python global? Just this line that
specifies
> the @type?

We could define a global macrodata object, extend it in pages and includes,
output it in json in a template.

Yes, @context, @type, anything else that is not page specific.

We should probably do this separately when we extend the macrodata in the
future.

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.t...
templates/default.tmpl:104: <? include meta/microdata ?>
On 2017/12/20 08:25:59, ire wrote:
> I guess that this could be considered metadata so perhaps we should put it in
the <head>?

That's what I had in mind. Not a strong opinion. But it seems to make more
sense.

[ire, 2018-01-03 14:01:18]

On 2017/12/20 11:50:17, juliandoucette wrote:
> Is there a reason that you chose to use the white logo instead of the black?

Total mistake! Fixed now.

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html
File pages/index.html (right):

https://codereview.adblockplus.org/29643557/diff/29643558/pages/index.html#ne...
pages/index.html:51: "@type": "Organization",
On 2017/12/20 11:50:16, juliandoucette wrote:
> On 2017/12/20 08:25:59, ire wrote:
> > Sorry, I don't quite understand. Since the data will be different for each
> page,
> > what exactly should we define in a python global? Just this line that
> specifies
> > the @type?
> 
> We could define a global macrodata object, extend it in pages and includes,
> output it in json in a template.
> 
> Yes, @context, @type, anything else that is not page specific.
> 
> We should probably do this separately when we extend the macrodata in the
> future.

Ack. Agreed.

[ire, 2018-01-03 14:01:42]

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.tmpl
File templates/default.tmpl (right):

https://codereview.adblockplus.org/29643557/diff/29643558/templates/default.t...
templates/default.tmpl:104: <? include meta/microdata ?>
On 2017/12/20 11:50:17, juliandoucette wrote:
> On 2017/12/20 08:25:59, ire wrote:
> > I guess that this could be considered metadata so perhaps we should put it
in
> the <head>?
> 
> That's what I had in mind. Not a strong opinion. But it seems to make more
> sense.

Done.

[juliandoucette, 2018-01-04 01:48:06]

LGTM