Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1329)

Issue 29593561: Issue 5961 - Add Imprint page to help.eyeo.com (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 8 months ago by ire
Modified:
1 year, 8 months ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

Issue 5961 - Add Imprint page to help.eyeo.com

Patch Set 1 #

Total comments: 20

Patch Set 2 : Organization schema, convert to html, remove unneeded translations #

Total comments: 9

Patch Set 3 : Update <hr> spacing, add comments, update Till itemprop #

Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -2 lines) Patch
M includes/layout/footer.tmpl View 1 chunk +1 line, -1 line 0 comments Download
A pages/legal.html View 1 2 1 chunk +75 lines, -0 lines 0 comments Download
M static/scss/base/_utilities.scss View 1 1 chunk +7 lines, -1 line 0 comments Download

Messages

Total messages: 8
ire
1 year, 8 months ago (2017-10-31 08:56:00 UTC) #1
ire
Ready
1 year, 8 months ago (2017-10-31 08:56:17 UTC) #2
ire
https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md File pages/legal.md (right): https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newcode20 pages/legal.md:20: {{ eyeo-address-street[address] Lichtstraße 25 }}<br> I separated this into ...
1 year, 8 months ago (2017-10-31 08:57:33 UTC) #3
juliandoucette
https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md File pages/legal.md (right): https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newcode2 pages/legal.md:2: NIT: No description? https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newcode12 pages/legal.md:12: <div class="column two-thirds" markdown="1"> ...
1 year, 8 months ago (2017-10-31 12:34:10 UTC) #4
ire
New patchset uploaded https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md File pages/legal.md (right): https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newcode2 pages/legal.md:2: On 2017/10/31 12:34:10, juliandoucette wrote: > ...
1 year, 8 months ago (2017-11-01 09:18:13 UTC) #5
ire
https://codereview.adblockplus.org/29593561/diff/29594558/pages/legal.html File pages/legal.html (right): https://codereview.adblockplus.org/29593561/diff/29594558/pages/legal.html#newcode68 pages/legal.html:68: <li itemprop="employee" itemscope itemtype="http://schema.org/Person"> I'm not sure about this, ...
1 year, 8 months ago (2017-11-01 09:19:12 UTC) #6
juliandoucette
LGTM + NITs https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md File pages/legal.md (right): https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newcode2 pages/legal.md:2: On 2017/11/01 09:18:13, ire wrote: > ...
1 year, 8 months ago (2017-11-01 13:09:47 UTC) #7
ire
1 year, 8 months ago (2017-11-02 09:20:55 UTC) #8
https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md
File pages/legal.md (right):

https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc...
pages/legal.md:2: 
On 2017/11/01 13:09:46, juliandoucette wrote:
> On 2017/11/01 09:18:13, ire wrote:
> > On 2017/10/31 12:34:10, juliandoucette wrote:
> > > NIT: No description?
> > 
> > There isn't one in the spec
> 
> NIT/Suggest: Ask for one.
> 
> (Can be addressed separately.)

https://issues.adblockplus.org/ticket/5968

https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc...
pages/legal.md:16: ---
On 2017/11/01 13:09:46, juliandoucette wrote:
> On 2017/11/01 09:18:12, ire wrote:
> > On 2017/10/31 12:34:10, juliandoucette wrote:
> > > NIT: I think these lines are presentational (and hr is supposed to be
> semantic
> > > in HTML5?)
> > 
> > I don't think they are purely presentational. They are separating different
> > sections of the content.
> 
> Ack. NIT: It looks like a heading followed by three sections with border-top
to
> me. 

But three sections means there are different sections of content :P Haha we may
have to agree to disagree on this one

https://codereview.adblockplus.org/29593561/diff/29594558/pages/legal.html
File pages/legal.html (right):

https://codereview.adblockplus.org/29593561/diff/29594558/pages/legal.html#ne...
pages/legal.html:8: hr { margin: 3em 0; }
On 2017/11/01 13:09:46, juliandoucette wrote:
> NIT/Suggest: This is too much space?

Done.

https://codereview.adblockplus.org/29593561/diff/29594558/pages/legal.html#ne...
pages/legal.html:17: .unstyled dd:after
On 2017/11/01 13:09:46, juliandoucette wrote:
> NIT/Suggest: Explain this clever solution via comment

Done :)

https://codereview.adblockplus.org/29593561/diff/29594558/pages/legal.html#ne...
pages/legal.html:68: <li itemprop="employee" itemscope
itemtype="http://schema.org/Person">
On 2017/11/01 13:09:46, juliandoucette wrote:
> On 2017/11/01 09:19:11, ire wrote:
> > I'm not sure about this, but I couldn't find any other title besides
> > founder/employee
> 
> I'm not sure either. My intuition says "founders" are different than "managing
> directors". And, although Till is both, he is an "employee" in the "Managing
> Directors" context.

Don't know why I didn't look this up before, but apparently we can have multiple
attributes - https://www.w3.org/TR/microdata/#x4-2-the-basic-syntax

So I'm setting Till as both a founder and employee

https://codereview.adblockplus.org/29593561/diff/29594558/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29593561/diff/29594558/static/scss/base/_u...
static/scss/base/_utilities.scss:73: .unstyled,
On 2017/11/01 13:09:47, juliandoucette wrote:
> Note: We should probably add this and below to wd (website-defaults) later.

Ack. I agree.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5