Issue 29593561: Issue 5961 - Add Imprint page to help.eyeo.com (Closed)
|
Created:
Oct. 31, 2017, 8:55 a.m. by ire Modified:
Nov. 2, 2017, 9:21 a.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/help.eyeo.com Visibility:
Public. |
DescriptionIssue 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 #
MessagesTotal messages: 8
Ready
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:20: {{ eyeo-address-street[address] Lichtstraße 25 }}<br> I separated this into different lines/translations because: 1. I wanted to keep this a markdown file 2. I couldn't find a way to separate the line (<br>) using this translation format.
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: NIT: No description? https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:12: <div class="column two-thirds" markdown="1"> Suggest: Organization schema (or similar, including sub-schemas e.g. address, contact info, people/roles) https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:16: --- NIT: I think these lines are presentational (and hr is supposed to be semantic in HTML5?) https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:20: {{ eyeo-address-street[address] Lichtstraße 25 }}<br> On 2017/10/31 08:57:33, ire wrote: > I separated this into different lines/translations because: > > 1. I wanted to keep this a markdown file > 2. I couldn't find a way to separate the line (<br>) using this translation > format. 1. Why? I don't expect this page to change or be edited often. 2. Our address doesn't have to be translated https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:22: {{ eyeo-address-country[address] Germany }} Suggest: <address> https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:26: {{ imprint-contact-forum For suppoprt please goto: [Adblock Plus Forum](https://adblockplus.org/forum/) }}<br> Suggest: description list https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:37: {{ till-faida[name] Till Faida }}<br> NIT: These names don't have to be translated Suggest: Unstyled, unordered, list
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#newc... pages/legal.md:2: On 2017/10/31 12:34:10, juliandoucette wrote: > NIT: No description? There isn't one in the spec https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:12: <div class="column two-thirds" markdown="1"> On 2017/10/31 12:34:10, juliandoucette wrote: > Suggest: Organization schema (or similar, including sub-schemas e.g. address, > contact info, people/roles) Good idea. Done. https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:16: --- 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. https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:20: {{ eyeo-address-street[address] Lichtstraße 25 }}<br> On 2017/10/31 12:34:10, juliandoucette wrote: > On 2017/10/31 08:57:33, ire wrote: > > I separated this into different lines/translations because: > > > > 1. I wanted to keep this a markdown file > > 2. I couldn't find a way to separate the line (<br>) using this translation > > format. > > 1. Why? I don't expect this page to change or be edited often. > 2. Our address doesn't have to be translated 1. Maybe not, but I figured that this is the sort of file we would want content editors to be able to change easily? 2. Probably not, but it's simpler to have everything in the translation format. I think we discussed and decided to translate everything on our end, and the decision can be made elsewhere if the text will actually be translated. https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:20: {{ eyeo-address-street[address] Lichtstraße 25 }}<br> On 2017/10/31 12:34:10, juliandoucette wrote: > On 2017/10/31 08:57:33, ire wrote: > > I separated this into different lines/translations because: > > > > 1. I wanted to keep this a markdown file > > 2. I couldn't find a way to separate the line (<br>) using this translation > > format. > > 1. Why? I don't expect this page to change or be edited often. > 2. Our address doesn't have to be translated Actually, since I am taking your suggestions to do the Organization schema, I might as well change this whole page to HTML. Also, I'm realising I probably over-translated here (e.g. translated phone numbers). I'm going to remove translations for more things. https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:22: {{ eyeo-address-country[address] Germany }} On 2017/10/31 12:34:10, juliandoucette wrote: > Suggest: <address> Done. https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:26: {{ imprint-contact-forum For suppoprt please goto: [Adblock Plus Forum](https://adblockplus.org/forum/) }}<br> On 2017/10/31 12:34:10, juliandoucette wrote: > Suggest: description list Done. https://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:37: {{ till-faida[name] Till Faida }}<br> On 2017/10/31 12:34:10, juliandoucette wrote: > NIT: These names don't have to be translated Now I'm not sure about my previous comment about always translating everything. Although I still think it cleaner. > Suggest: Unstyled, unordered, list Done.
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:68: <li itemprop="employee" itemscope itemtype="http://schema.org/Person"> I'm not sure about this, but I couldn't find any other title besides founder/employee
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#newc... pages/legal.md:2: 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://codereview.adblockplus.org/29593561/diff/29593562/pages/legal.md#newc... pages/legal.md:16: --- 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. 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; } NIT/Suggest: This is too much space? https://codereview.adblockplus.org/29593561/diff/29594558/pages/legal.html#ne... pages/legal.html:17: .unstyled dd:after NIT/Suggest: Explain this clever solution via comment 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 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. 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, Note: We should probably add this and below to wd (website-defaults) later.
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. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
