Issue 5672 - Misplaced content on adblockplus.org/maxthon

Issue 5672 - Misplaced content on adblockplus.org/maxthon

[ire, 2017-09-14 08:11:50]

Ready for review.

There were a couple other issues I noticed and fixed (comments below). I can fix
them in a separate noissue if that's preferred.

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl...
includes/index.tmpl:132: <img id="maxthon-instruction"
src="/img/maxthon-instruction.png" alt="{{"Activate Adblock Plus in
Maxthon"|translate("maxthon-instruction")}}">
Alt text was missing for this image

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl...
includes/index.tmpl:133: <img id="edge-teaser" src="/img/edge_teaser.png"
alt="{{"Comming soon image of Adblock Plus for Microsoft
Edge"|translate("edge-teaser")}}" />
I noticed that the alt text here was missing quotes and causing it to not be
displayed properly, so I added them.

[ire, 2017-09-15 09:02:51]

Adding @greiner and @saroyanm . If either of you have time to review this before
Julian is back from holiday, it would be appreciated :)

[Thomas Greiner, 2017-09-15 13:48:30]

LGTM with just small details. But note that I'm not a peer of the Websites
module so best to wait for Manvel's approval.

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl...
includes/index.tmpl:132: <img id="maxthon-instruction"
src="/img/maxthon-instruction.png" alt="{{"Activate Adblock Plus in
Maxthon"|translate("maxthon-instruction")}}">
On 2017/09/14 08:11:50, ire wrote:
> Alt text was missing for this image

Acknowledged.

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl...
includes/index.tmpl:133: <img id="edge-teaser" src="/img/edge_teaser.png"
alt="{{"Comming soon image of Adblock Plus for Microsoft
Edge"|translate("edge-teaser")}}" />
On 2017/09/14 08:11:50, ire wrote:
> I noticed that the alt text here was missing quotes and causing it to not be
> displayed properly, so I added them.

Acknowledged.

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl...
includes/index.tmpl:133: <img id="edge-teaser" src="/img/edge_teaser.png"
alt="{{"Comming soon image of Adblock Plus for Microsoft
Edge"|translate("edge-teaser")}}" />
Typo: Replace "Comming" with "Coming"

https://codereview.adblockplus.org/29544557/diff/29544558/static/css/index-de...
File static/css/index-desktop.css (right):

https://codereview.adblockplus.org/29544557/diff/29544558/static/css/index-de...
static/css/index-desktop.css:19: min-height: 310px; /* Force height to be at
least the height of the video/image */
Coding style: Line length should be 80 characters or less

[ire, 2017-09-16 09:50:41]

> LGTM with just small details. But note that I'm not a peer of the Websites
> module so best to wait for Manvel's approval.

Thanks Thomas! I've addressed your notes, but will wait for a LGTM from
Julian/Manvel before I push.

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29544557/diff/29544558/includes/index.tmpl...
includes/index.tmpl:133: <img id="edge-teaser" src="/img/edge_teaser.png"
alt="{{"Comming soon image of Adblock Plus for Microsoft
Edge"|translate("edge-teaser")}}" />
On 2017/09/15 13:48:30, Thomas Greiner wrote:
> Typo: Replace "Comming" with "Coming"

Done.

https://codereview.adblockplus.org/29544557/diff/29544558/static/css/index-de...
File static/css/index-desktop.css (right):

https://codereview.adblockplus.org/29544557/diff/29544558/static/css/index-de...
static/css/index-desktop.css:19: min-height: 310px; /* Force height to be at
least the height of the video/image */
On 2017/09/15 13:48:30, Thomas Greiner wrote:
> Coding style: Line length should be 80 characters or less

Done.

[juliandoucette, 2017-09-17 22:39:03]

You may push this Patchset without the [comment, and edge typo fix (which I
suggest you push and sync separately)] if you create a separate issue to replace
the YouTube video with the infographic as originally intended. I will address
the layout issue in a design update of abp.org ASAP.

https://codereview.adblockplus.org/29544557/diff/29546555/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29544557/diff/29546555/includes/index.tmpl...
includes/index.tmpl:132: <img id="maxthon-instruction"
src="/img/maxthon-instruction.png" alt="{{"Activate Adblock Plus in
Maxthon"|translate("maxthon-instruction")}}">
This image is supposed to replace the YouTube video. See
https://issues.adblockplus.org/ticket/1817

https://codereview.adblockplus.org/29544557/diff/29546555/includes/index.tmpl...
includes/index.tmpl:133: <img id="edge-teaser" src="/img/edge_teaser.png"
alt="{{"Coming soon image of Adblock Plus for Microsoft
Edge"|translate("edge-teaser")}}" />
NIT: This should be updated separately and synced "without changes" to avoid
invalidating translations.

https://codereview.adblockplus.org/29544557/diff/29546555/static/css/index-de...
File static/css/index-desktop.css (right):

https://codereview.adblockplus.org/29544557/diff/29546555/static/css/index-de...
static/css/index-desktop.css:19: /* Force height to be at least the height of
the video/image */
I'd rather document this in an issue to be fixed than a comment. This section
(the YouTube video, and content beside it) is laid out poorly IMO.

[juliandoucette, 2017-09-17 22:44:20]

On 2017/09/17 22:39:03, juliandoucette wrote:

Please ignore my request to remove / push without the comment. The comment makes
sense / is fine for now (and we should not create another issue) because I will
update this layout when I update abp.org anyway. Sorry for the confusion! :)

[ire, 2017-09-18 07:47:38]

Thanks Julian. Just to be clear, all I did was revert the fix of the edge-teaser
image alternative text. Is there anything else needed for this issue?

https://codereview.adblockplus.org/29544557/diff/29546555/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29544557/diff/29546555/includes/index.tmpl...
includes/index.tmpl:132: <img id="maxthon-instruction"
src="/img/maxthon-instruction.png" alt="{{"Activate Adblock Plus in
Maxthon"|translate("maxthon-instruction")}}">
On 2017/09/17 22:39:02, juliandoucette wrote:
> This image is supposed to replace the YouTube video. See
> https://issues.adblockplus.org/ticket/1817

Acknowledged.

https://codereview.adblockplus.org/29544557/diff/29546555/includes/index.tmpl...
includes/index.tmpl:133: <img id="edge-teaser" src="/img/edge_teaser.png"
alt="{{"Coming soon image of Adblock Plus for Microsoft
Edge"|translate("edge-teaser")}}" />
On 2017/09/17 22:39:02, juliandoucette wrote:
> NIT: This should be updated separately and synced "without changes" to avoid
> invalidating translations.

Done.

[juliandoucette, 2017-09-18 10:45:47]

On 2017/09/18 07:47:38, ire wrote:
> Thanks Julian. Just to be clear, all I did was revert the fix of the
edge-teaser
> image alternative text. Is there anything else needed for this issue?

No. Not unless you want to address the YouTube video vs Infographic in this
issue (which is not necessary). LGTM.