Issue 2157 - Update Large Scale Deployments landing page

Related ticket:
https://issues.adblockplus.org/ticket/2157

[saroyanm, 2015-06-11 14:02:38]

@Sebastian can you please have a look ? 
Actually I had a question which I commented in the review. I will really
appreciate your input on that implementation detail which i missed while marking
the ticket as ready.

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:41: <div class="toc">
@Sebastian I just hard-coded this here to make it more descriptive on the
example, I'm not sure whether I should create separate ticket for implementing
toc that could be placed anywhere on the page, will appreciate your input here.
What you think how it would be better to implement the TOC on this page so it
could be placed after contact details ?

[Sebastian Noack, 2015-06-11 14:34:00]

Isn't the raw HTML format deprecated? Then this would probably be the right time
to convert that page to jinja2.

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:41: <div class="toc">
On 2015/06/11 14:02:38, saroyanm wrote:
> @Sebastian I just hard-coded this here to make it more descriptive on the
> example, I'm not sure whether I should create separate ticket for implementing
> toc that could be placed anywhere on the page, will appreciate your input
here.
> What you think how it would be better to implement the TOC on this page so it
> could be placed after contact details ?

So you did hard-code the toc to not have it at the top of the page as usual? Is
it generally a good idea to introduce this inconsistency? Or do I miss the
point?

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:43: <li><a href="#what-is-lsd">{{what-is-lsd-header What
are large scale deployments?}}</a></li>
Since this page is specific to large scale deployments I don't think you need to
add "lsd" to all message ids here.

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
File locales/de/deployments.json (right):

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
locales/de/deployments.json:18: "description": "Feature - Save bandwidth"
I feel that most (if not all) descriptions are meaningless, as they merely
repeat the message id and/or source string. Also note that message IDs in target
translations wouldn't be uploaded to CrowdIn.

[saroyanm, 2015-06-11 14:44:29]

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:41: <div class="toc">
On 2015/06/11 14:34:00, Sebastian Noack wrote:
> On 2015/06/11 14:02:38, saroyanm wrote:
> > @Sebastian I just hard-coded this here to make it more descriptive on the
> > example, I'm not sure whether I should create separate ticket for
implementing
> > toc that could be placed anywhere on the page, will appreciate your input
> here.
> > What you think how it would be better to implement the TOC on this page so
it
> > could be placed after contact details ?
> 
> So you did hard-code the toc to not have it at the top of the page as usual?
Is
> it generally a good idea to introduce this inconsistency? Or do I miss the
> point?

Well actually this is something that probably needs to be changed as I mentioned
I've hardcoded to show you the problem, as you can see in the mockup TOC is
placed after contact details:
https://issues.adblockplus.org/attachment/ticket/2157/LSD_Mockup_02.png 

So the current Mockup is inconsistent to what we currently have in CMS. 
I'm not sure whether make sense to update the mockup or implement Custom TOC
that can be placed anyway in the page. I would actually vote for second one,
because if we will update the mockup we should also need to consider the
placement of LSD icon next to title.

[Sebastian Noack, 2015-06-11 14:53:58]

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:41: <div class="toc">
On 2015/06/11 14:44:29, saroyanm wrote:
> On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > On 2015/06/11 14:02:38, saroyanm wrote:
> > > @Sebastian I just hard-coded this here to make it more descriptive on the
> > > example, I'm not sure whether I should create separate ticket for
> implementing
> > > toc that could be placed anywhere on the page, will appreciate your input
> > here.
> > > What you think how it would be better to implement the TOC on this page so
> it
> > > could be placed after contact details ?
> > 
> > So you did hard-code the toc to not have it at the top of the page as usual?
> Is
> > it generally a good idea to introduce this inconsistency? Or do I miss the
> > point?
> 
> Well actually this is something that probably needs to be changed as I
mentioned
> I've hardcoded to show you the problem, as you can see in the mockup TOC is
> placed after contact details:
> https://issues.adblockplus.org/attachment/ticket/2157/LSD_Mockup_02.png 
> 
> So the current Mockup is inconsistent to what we currently have in CMS. 
> I'm not sure whether make sense to update the mockup or implement Custom TOC
> that can be placed anyway in the page. I would actually vote for second one,
> because if we will update the mockup we should also need to consider the
> placement of LSD icon next to title. 

Yeah, that's what I mean. And yes, we should either get rid of this
inconsistency moving the TOC to the top of the page, or add a mechanism to
generate the TOC here.

I don't mind. So I looped in Sven and Thomas.

[saroyanm, 2015-06-11 16:27:48]

New patch uploaded.

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:43: <li><a href="#what-is-lsd">{{what-is-lsd-header What
are large scale deployments?}}</a></li>
On 2015/06/11 14:34:00, Sebastian Noack wrote:
> Since this page is specific to large scale deployments I don't think you need
to
> add "lsd" to all message ids here.

I get rid of some of them, but sometimes it's descriptive I think for example in
this case what-is-lsd make more sense.

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
File locales/de/deployments.json (right):

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
locales/de/deployments.json:18: "description": "Feature - Save bandwidth"
On 2015/06/11 14:34:00, Sebastian Noack wrote:
> I feel that most (if not all) descriptions are meaningless, as they merely
> repeat the message id and/or source string. Also note that message IDs in
target
> translations wouldn't be uploaded to CrowdIn.

Updated, please let me know whether now it's less meaningless :) 
Actually repeating source string were helping me to translate through CMS, but
you are right I should think to make it useful for CrowdIn translators, please
let me know if the descriptions still meaningless.

[Sebastian Noack, 2015-06-12 11:22:58]

On 2015/06/11 14:34:00, Sebastian Noack wrote:
> Isn't the raw HTML format deprecated? Then this would probably be the right
time
> to convert that page to jinja2.

Also mind this comment?

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:43: <li><a href="#what-is-lsd">{{what-is-lsd-header What
are large scale deployments?}}</a></li>
On 2015/06/11 16:27:48, saroyanm wrote:
> On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > Since this page is specific to large scale deployments I don't think you
need
> to
> > add "lsd" to all message ids here.
> 
> I get rid of some of them, but sometimes it's descriptive I think for example
in
> this case what-is-lsd make more sense.

Well, you could simply change "what-is-lsd" in "about". And with most other like
"lsd-header-block" and "lsd-icon-wrapper" I don't see why you didn't simply
remove "lsd" there.

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
File locales/de/deployments.json (right):

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
locales/de/deployments.json:18: "description": "Feature - Save bandwidth"
On 2015/06/11 16:27:48, saroyanm wrote:
> On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > I feel that most (if not all) descriptions are meaningless, as they merely
> > repeat the message id and/or source string. Also note that message IDs in
> target
> > translations wouldn't be uploaded to CrowdIn.
> 
> Updated, please let me know whether now it's less meaningless :) 
> Actually repeating source string were helping me to translate through CMS, but
> you are right I should think to make it useful for CrowdIn translators, please
> let me know if the descriptions still meaningless.

Sorry, there were a typo in my comment I meant that descriptions in target
translation wouldn't be uploaded to CrowdIn. What I suggested was to remove the
descriptions from all translations.

[saroyanm, 2015-06-12 12:03:07]

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:43: <li><a href="#what-is-lsd">{{what-is-lsd-header What
are large scale deployments?}}</a></li>
On 2015/06/12 11:22:58, Sebastian Noack wrote:
> On 2015/06/11 16:27:48, saroyanm wrote:
> > On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > > Since this page is specific to large scale deployments I don't think you
> need
> > to
> > > add "lsd" to all message ids here.
> > 
> > I get rid of some of them, but sometimes it's descriptive I think for
example
> in
> > this case what-is-lsd make more sense.
> 
> Well, you could simply change "what-is-lsd" in "about". And with most other
like
> "lsd-header-block" and "lsd-icon-wrapper" I don't see why you didn't simply
> remove "lsd" there.

lsd-header-block and lsd-icon-wrapper are IDs, please note that we are loading
also other CSS styles here, so in future for example if we will have
header-block ID in header then we could override that rule apart of having
multiple IDs with same name, so that why I prefer some kind of prefix in the IDs
and Class names for this page.
Sure in case we will have several page with similar styles we should somehow
change the styles to some generic name.

[Sebastian Noack, 2015-06-12 12:07:34]

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:43: <li><a href="#what-is-lsd">{{what-is-lsd-header What
are large scale deployments?}}</a></li>
On 2015/06/12 12:03:07, saroyanm wrote:
> On 2015/06/12 11:22:58, Sebastian Noack wrote:
> > On 2015/06/11 16:27:48, saroyanm wrote:
> > > On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > > > Since this page is specific to large scale deployments I don't think you
> > need
> > > to
> > > > add "lsd" to all message ids here.
> > > 
> > > I get rid of some of them, but sometimes it's descriptive I think for
> example
> > in
> > > this case what-is-lsd make more sense.
> > 
> > Well, you could simply change "what-is-lsd" in "about". And with most other
> like
> > "lsd-header-block" and "lsd-icon-wrapper" I don't see why you didn't simply
> > remove "lsd" there.
> 
> lsd-header-block and lsd-icon-wrapper are IDs, please note that we are loading
> also other CSS styles here, so in future for example if we will have
> header-block ID in header then we could override that rule apart of having
> multiple IDs with same name, so that why I prefer some kind of prefix in the
IDs
> and Class names for this page.
> Sure in case we will have several page with similar styles we should somehow
> change the styles to some generic name.

You are right, my bad. I were only talking about message IDs which aren't shared
across pages, hence shouldn't mention lsd here.

[saroyanm, 2015-06-12 13:46:04]

New patch uploaded

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
File pages/deployments.html (right):

http://codereview.adblockplus.org/6275268058546176/diff/5629499534213120/page...
pages/deployments.html:43: <li><a href="#what-is-lsd">{{what-is-lsd-header What
are large scale deployments?}}</a></li>
On 2015/06/12 12:07:35, Sebastian Noack wrote:
> On 2015/06/12 12:03:07, saroyanm wrote:
> > On 2015/06/12 11:22:58, Sebastian Noack wrote:
> > > On 2015/06/11 16:27:48, saroyanm wrote:
> > > > On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > > > > Since this page is specific to large scale deployments I don't think
you
> > > need
> > > > to
> > > > > add "lsd" to all message ids here.
> > > > 
> > > > I get rid of some of them, but sometimes it's descriptive I think for
> > example
> > > in
> > > > this case what-is-lsd make more sense.
> > > 
> > > Well, you could simply change "what-is-lsd" in "about". And with most
other
> > like
> > > "lsd-header-block" and "lsd-icon-wrapper" I don't see why you didn't
simply
> > > remove "lsd" there.
> > 
> > lsd-header-block and lsd-icon-wrapper are IDs, please note that we are
loading
> > also other CSS styles here, so in future for example if we will have
> > header-block ID in header then we could override that rule apart of having
> > multiple IDs with same name, so that why I prefer some kind of prefix in the
> IDs
> > and Class names for this page.
> > Sure in case we will have several page with similar styles we should somehow
> > change the styles to some generic name.
> 
> You are right, my bad. I were only talking about message IDs which aren't
shared
> across pages, hence shouldn't mention lsd here.

Done.

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
File locales/de/deployments.json (right):

http://codereview.adblockplus.org/6275268058546176/diff/5105650963054592/loca...
locales/de/deployments.json:18: "description": "Feature - Save bandwidth"
On 2015/06/12 11:22:58, Sebastian Noack wrote:
> On 2015/06/11 16:27:48, saroyanm wrote:
> > On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > > I feel that most (if not all) descriptions are meaningless, as they merely
> > > repeat the message id and/or source string. Also note that message IDs in
> > target
> > > translations wouldn't be uploaded to CrowdIn.
> > 
> > Updated, please let me know whether now it's less meaningless :) 
> > Actually repeating source string were helping me to translate through CMS,
but
> > you are right I should think to make it useful for CrowdIn translators,
please
> > let me know if the descriptions still meaningless.
> 
> Sorry, there were a typo in my comment I meant that descriptions in target
> translation wouldn't be uploaded to CrowdIn. What I suggested was to remove
the
> descriptions from all translations. 

Ahh, now I got it.

[Sebastian Noack, 2015-06-12 17:20:17]

On 2015/06/12 11:22:58, Sebastian Noack wrote:
> On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > Isn't the raw HTML format deprecated? Then this would probably be the right
> time
> > to convert that page to jinja2.
> 
> Also mind this comment?

And yet again, what's about this one? ;)

[saroyanm, 2015-06-13 13:48:16]

On 2015/06/12 17:20:17, Sebastian Noack wrote:
> On 2015/06/12 11:22:58, Sebastian Noack wrote:
> > On 2015/06/11 14:34:00, Sebastian Noack wrote:
> > > Isn't the raw HTML format deprecated? Then this would probably be the
right
> > time
> > > to convert that page to jinja2.
> > 
> > Also mind this comment?
> 
> And yet again, what's about this one? ;)

OOps went out of my radar.

[saroyanm, 2015-06-13 13:48:36]

New patches uploaded.

[Sebastian Noack, 2015-06-13 17:16:51]

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
File pages/deployments.tmpl (right):

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:7: <meta name="title" content="{{'Large Scale
Deployment'|translate('meta-title')}}" />
Nit: Please double quotes consistently, unless there is a reason (e.g. double
quotes inside quoted strings).

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:7: <meta name="title" content="{{'Large Scale
Deployment'|translate('meta-title')}}" />
I presonally find it a little easier to read if putting a space after opening
and before closing jinja expressions, e.g.:

  {{ '..'|translate(..) }}

But given that we already do it inconsistently, I leave it up to you.

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:67: {{"\"With extremely limited bandwidth here in remote
Alaska, I find that every bit that travels over the satellite
counts.\""|translate("unalaska-school-quote")}}
I'd move the quotes out of the translation. This won't only make the string here
slightly more readable since no escaping would be required, but also eliminates
the risk of translators accidentally removing the quotes.

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:70: <i>- {{"Brent Bain, <a
href=\"http://ucsd.net/\">Unalaska City School
District</a>"|translate("unalaska-school-author")}}</i>
I really think we shouldn't translate their names and organizations.

[saroyanm, 2015-06-14 10:26:53]

New patch uploaded.

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
File pages/deployments.tmpl (right):

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:7: <meta name="title" content="{{'Large Scale
Deployment'|translate('meta-title')}}" />
On 2015/06/13 17:16:51, Sebastian Noack wrote:
> Nit: Please double quotes consistently, unless there is a reason (e.g. double
> quotes inside quoted strings).

Done.

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:7: <meta name="title" content="{{'Large Scale
Deployment'|translate('meta-title')}}" />
On 2015/06/13 17:16:51, Sebastian Noack wrote:
> I presonally find it a little easier to read if putting a space after opening
> and before closing jinja expressions, e.g.:
> 
>   {{ '..'|translate(..) }}
> 
> But given that we already do it inconsistently, I leave it up to you. 

Done. I'll follow this style for future.

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:67: {{"\"With extremely limited bandwidth here in remote
Alaska, I find that every bit that travels over the satellite
counts.\""|translate("unalaska-school-quote")}}
On 2015/06/13 17:16:51, Sebastian Noack wrote:
> I'd move the quotes out of the translation. This won't only make the string
here
> slightly more readable since no escaping would be required, but also
eliminates
> the risk of translators accidentally removing the quotes.

Good point, done.

http://codereview.adblockplus.org/6275268058546176/diff/5167242941562880/page...
pages/deployments.tmpl:70: <i>- {{"Brent Bain, <a
href=\"http://ucsd.net/\">Unalaska City School
District</a>"|translate("unalaska-school-author")}}</i>
On 2015/06/13 17:16:51, Sebastian Noack wrote:
> I really think we shouldn't translate their names and organizations.

Done.

[Sebastian Noack, 2015-06-14 11:44:29]

LGTM