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

Issue 29722640: Fixes #22 - Add download page (Closed)

Created:
March 14, 2018, 1:31 p.m. by ire
Modified:
April 3, 2018, 3:41 p.m.
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

Fixes #22 - Add download page Issue - https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/22 Branch - index_page

Patch Set 1 #

Total comments: 29

Patch Set 2 : Addressed comments #3 #

Total comments: 21

Patch Set 3 : Resize abb logo, use linkify function #

Patch Set 4 : Rebase #

Total comments: 15

Patch Set 5 : Addressed comments #17 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -0 lines) Patch
A pages/download.tmpl View 1 2 3 4 1 chunk +94 lines, -0 lines 0 comments Download
M static/css/main.css View 1 2 3 4 1 chunk +21 lines, -0 lines 0 comments Download
A static/img/abb-icon.png View 1 2 3 4 Binary file 0 comments Download
A static/img/abb-icon-2x.png View 1 2 3 4 Binary file 0 comments Download
A static/img/device-desktop.png View Binary file 0 comments Download
A static/img/device-desktop-2x.png View Binary file 0 comments Download
A static/img/device-mobile.png View Binary file 0 comments Download
A static/img/device-mobile.svg View 1 chunk +3 lines, -0 lines 0 comments Download
A static/img/download-icon.png View Binary file 0 comments Download
A static/img/download-icon.svg View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 21
ire
March 14, 2018, 1:31 p.m. (2018-03-14 13:31:42 UTC) #1
ire
Hey Julian! I'm waiting for the features section review to be done to update that, ...
March 14, 2018, 1:34 p.m. (2018-03-14 13:34:37 UTC) #2
juliandoucette
https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl#newcode2 pages/download.tmpl:2: description=Get Adblock Plus for free on Firefox, Chrome, Opera, ...
March 14, 2018, 5:54 p.m. (2018-03-14 17:54:53 UTC) #3
ire
New patch set https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29722641/pages/download.tmpl#newcode19 pages/download.tmpl:19: "description": "By clicking any of the ...
March 15, 2018, 9:38 a.m. (2018-03-15 09:38:12 UTC) #4
ire
@Manvel since Julian is away could you please have a look at this for me? ...
March 16, 2018, 9:12 a.m. (2018-03-16 09:12:24 UTC) #5
saroyanm
On 2018/03/16 09:12:24, ire wrote: > @Manvel since Julian is away could you please have ...
March 16, 2018, 3:48 p.m. (2018-03-16 15:48:34 UTC) #6
saroyanm
On 2018/03/16 15:48:34, saroyanm wrote: > On 2018/03/16 09:12:24, ire wrote: > > @Manvel since ...
March 19, 2018, 2:02 p.m. (2018-03-19 14:02:21 UTC) #7
saroyanm
Thanks Ire, first round is ready. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl#newcode1 pages/download.tmpl:1: title=Download Adblock Plus ...
March 19, 2018, 7:04 p.m. (2018-03-19 19:04:07 UTC) #8
ire
Thanks Manvel! New patch uploaded I've moved some of your comments here https://gitlab.com/eyeo/specs/spec/merge_requests/121#note_64068129 so they ...
March 20, 2018, 7:25 a.m. (2018-03-20 07:25:27 UTC) #9
ire
On 2018/03/20 07:25:27, ire wrote: > I've moved some of your comments here > https://gitlab.com/eyeo/specs/spec/merge_requests/121#note_64068129 ...
March 20, 2018, 7:27 a.m. (2018-03-20 07:27:23 UTC) #10
ire
@Manvel responses from Jeen and Tamara : https://gitlab.com/eyeo/specs/spec/merge_requests/135 https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl#newcode22 pages/download.tmpl:22: { ...
March 20, 2018, 2:40 p.m. (2018-03-20 14:40:59 UTC) #11
saroyanm
Sorry for late reply, was busy with the ABPUI release process. https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): ...
March 23, 2018, 6:33 p.m. (2018-03-23 18:33:39 UTC) #12
saroyanm
Sorry for late reply, was busy with the ABPUI release process.
March 23, 2018, 6:33 p.m. (2018-03-23 18:33:42 UTC) #13
ire
https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl#newcode22 pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, On 2018/03/23 18:33:38, ...
March 26, 2018, 7:29 a.m. (2018-03-26 07:29:08 UTC) #14
ire
https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl File pages/download.tmpl (right): https://codereview.adblockplus.org/29722640/diff/29723573/pages/download.tmpl#newcode22 pages/download.tmpl:22: { "name": "Chrome", "url": "https://chrome.google.com/webstore/detail/cfhdojbkjhnklbpkdaibdccddilifddb" }, On 2018/03/26 07:29:07, ...
March 26, 2018, 9:44 a.m. (2018-03-26 09:44:28 UTC) #15
ire
On 2018/03/23 18:33:42, saroyanm wrote: > Sorry for late reply, was busy with the ABPUI ...
March 26, 2018, 10:23 a.m. (2018-03-26 10:23:24 UTC) #16
juliandoucette
LGTM + NITs Don't forget to optimize images if they aren't already. https://codereview.adblockplus.org/29722640/diff/29729581/includes/features.html File includes/features.html ...
April 3, 2018, 1:24 p.m. (2018-04-03 13:24:52 UTC) #17
juliandoucette
RE: "uh...?" - I'm not sure if you meant to include the item-group change in ...
April 3, 2018, 1:27 p.m. (2018-04-03 13:27:10 UTC) #18
ire
On 2018/04/03 13:27:10, juliandoucette wrote: > RE: "uh...?" - I'm not sure if you meant ...
April 3, 2018, 3:03 p.m. (2018-04-03 15:03:41 UTC) #19
ire
On 2018/04/03 15:03:41, ire wrote: > On 2018/04/03 13:27:10, juliandoucette wrote: > > RE: "uh...?" ...
April 3, 2018, 3:15 p.m. (2018-04-03 15:15:14 UTC) #20
ire
April 3, 2018, 3:37 p.m. (2018-04-03 15:37:38 UTC) #21
Thanks Julian! I've addressed your comments and will push

https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl
File pages/download.tmpl (right):

https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl...
pages/download.tmpl:22: "description": "By clicking any of the links below, you
agree to our " +  "terms" | linkify  + "Terms of Use</a>.",
On 2018/04/03 13:24:51, juliandoucette wrote:
> NIT/Suggest: <small> (semantic, not size.)
> 
> > The HTML <small> element makes the text font size one size smaller (for
> example, from large to medium, or from small to x-small) down to the browser's
> minimum font size.  In HTML5, this element is repurposed to represent
> side-comments and small print, including copyright and legal text, independent
> of its styled presentation.
> >
> > -- mdn
> 

Ack. Done.

https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl...
pages/download.tmpl:72: <div class="column one-third">
On 2018/04/03 13:24:51, juliandoucette wrote:
> NIT/Suggest: <article>
> 
> > The HTML <article> element represents a self-contained composition in a
> document, page, application, or site, which is intended to be independently
> distributable or reusable (e.g., in syndication). Examples include: a forum
> post, a magazine or newspaper article, or a blog entry.
> >
> > -- mdn

I don't think this fits within the definition of an <article>

I don't think it is self-contained. Each "device" seems to be to be part of the
rest of the page, under the page title "Get Adblock Plus products on these
devices". I don't think each device would be considered to be standalone.

https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl...
pages/download.tmpl:77: srcset="/img/{{ device.icons[1] }} 2x"
On 2018/04/03 13:24:51, juliandoucette wrote:
> NIT: You could use a dict and place device.icons.1x and 2x to be more
> descriptive.

Done.

https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl...
pages/download.tmpl:78: alt="{{ device.icon_alt | translate(device.id +
"-icon-alt", "Alt text") }}">
On 2018/04/03 13:24:51, juliandoucette wrote:
> NIT: I usually imagine "id"s to be for machines and "name"s to be for humans.
> And it seems like you're referring to a human kind of "id" (a "name").  I
think
> this is a PHP convention. You may consider adopting it if you like the idea.

I look at "id"s simply as "identifiers". I wouldn't consider "abb" for example,
to be a name, it's a simplified version of the name that I'm using an an
identifier.

https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl...
pages/download.tmpl:79: <h2>{{ device.title | translate(device.id + "-title",
"Heading") }}</h2>
On 2018/04/03 13:24:50, juliandoucette wrote:
> NIT: It's unnecessary to capitalize string contexts.
> 
> (The same applies elsewhere.)

I like it capitalised. It conveys to me that it is more of a
sentence/description rather than a single word/identifier.

https://codereview.adblockplus.org/29722640/diff/29729581/pages/download.tmpl...
pages/download.tmpl:88: <a href="{{ platform.url }}">{{ platform.name |
translate(platform_identifier, "Product name") }}</a>{{ ", " if loop.index !=
loop.length }}
On 2018/04/03 13:24:51, juliandoucette wrote:
> Cool :+1:

Thanks :p

Powered by Google App Engine
This is Rietveld