Issue 29490624: Issue 5373 - Remove the "blog" from acceptableads.com and link to blog.acceptableads.com

Issue 29490624: Issue 5373 - Remove the "blog" from acceptableads.com and link to blog.acceptableads.com (Closed)

Created:
July 17, 2017, 12:42 p.m. by ire

Modified:
Aug. 1, 2017, 10:06 a.m.

Reviewers:
juliandoucette

CC:
ben, jobp, wspee, jeen

Base URL:
https://hg.adblockplus.org/web.acceptableads.com

Visibility:
Public.

Description

Issue 5373 - Remove the "blog" from acceptableads.com and link to blog.acceptableads.com

Patch Set 2 : Translate alt text, Fix icon size and colour #

Patch Set 3 : Remove rel attribute #

Total comments: 6

Patch Set 4 : Addressed NITs #

Created: July 20, 2017, 12:37 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+48 lines, -323 lines)			Patch
M	README.md	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	globals/sitemap.py	View		1 chunk	+1 line, -1 line	0 comments	Download
R	includes/blog/all-posts.tmpl	View		1 chunk	+0 lines, -29 lines	0 comments	Download
R	includes/blog/recent-aac.tmpl	View		1 chunk	+0 lines, -23 lines	0 comments	Download
M	includes/breadcrumbs.tmpl	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	includes/sidebar/primary-navigation.tmpl	View	1 2 3	1 chunk	+21 lines, -12 lines	0 comments	Download
R	pages/blog/first-update.md	View		1 chunk	+0 lines, -28 lines	0 comments	Download
R	pages/blog/independent-committee.md	View		1 chunk	+0 lines, -67 lines	0 comments	Download
R	pages/blog/index.md	View		1 chunk	+0 lines, -25 lines	0 comments	Download
R	pages/blog/second-update.md	View		1 chunk	+0 lines, -25 lines	0 comments	Download
R	pages/blog/third-update.md	View		1 chunk	+0 lines, -34 lines	0 comments	Download
R	pages/blog/welcome.md	View		1 chunk	+0 lines, -21 lines	0 comments	Download
M	pages/committee/index.html	View		1 chunk	+0 lines, -2 lines	0 comments	Download
M	static/css/main.css	View	1 2 3	1 chunk	+9 lines, -0 lines	0 comments	Download
A	static/img/png/icon-external-link.png	View	1	Binary file		0 comments	Download
A	static/img/svg/icon-external-link.svg	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	static/scss/layout/_sidebar.scss	View	1 2 3	2 chunks	+13 lines, -0 lines	0 comments	Download
R	templates/blog-entry.tmpl	View		1 chunk	+0 lines, -54 lines	0 comments	Download

Messages

Total messages: 8

Expand All Messages | Collapse All Messages

ire

https://codereview.adblockplus.org/29490624/diff/29490625/includes/sidebar/primary-navigation.tmpl File includes/sidebar/primary-navigation.tmpl (right): https://codereview.adblockplus.org/29490624/diff/29490625/includes/sidebar/primary-navigation.tmpl#newcode23 includes/sidebar/primary-navigation.tmpl:23: <a href="{{ sitemap_page }}" target="_blank" rel="noopener noreferrer"> Not sure ...

July 17, 2017, 12:46 p.m. (2017-07-17 12:46:20 UTC) #2

juliandoucette

Minor issues :) --- Thanks for the info about the noopener/noreferrer... I didn't know about ...

July 18, 2017, 4:58 p.m. (2017-07-18 16:58:50 UTC) #3

ire

https://codereview.adblockplus.org/29490624/diff/29490625/README.md File README.md (right): https://codereview.adblockplus.org/29490624/diff/29490625/README.md#newcode10 README.md:10: - External Link Icon: [CC BY 3.0 US](http://creativecommons.org/licenses/by/3.0/us/), [Raphaël ...

July 19, 2017, 12:55 p.m. (2017-07-19 12:55:22 UTC) #4

juliandoucette

July 19, 2017, 3:03 p.m. (2017-07-19 15:03:32 UTC) #5

ire

July 19, 2017, 6:10 p.m. (2017-07-19 18:10:11 UTC) #6

https://codereview.adblockplus.org/29490624/diff/29490625/includes/sidebar/pr...
File includes/sidebar/primary-navigation.tmpl (right):

https://codereview.adblockplus.org/29490624/diff/29490625/includes/sidebar/pr...
includes/sidebar/primary-navigation.tmpl:23: <a href="{{ sitemap_page }}"
target="_blank" rel="noopener noreferrer">
On 2017/07/19 15:03:31, juliandoucette wrote:
> On 2017/07/19 12:55:22, ire wrote:
> > From what I can tell it seems like a good idea to, as a default, apply this
to
> > external links. In this case, perhaps the noreferrer value should be
excluded
> > since we are linking to our own blog and may want the stats. Regarding the
> > nofollow value, you're probably right that we can trust http://medium.com
not
> to attack
> > us. However, there may still be some performance benefits to be gained from
> > using it. What do you think? Is it worth including at all?
> 
> - I want to exclude `noreferrer` in this case
> - I want to keep `noopener` in general
> 
> By this logic, we should add both of these `rel`s to `a`s by default and
exclude
> `noreferrer` if the `href` is whitelisted. But I think it's a better idea to
do
> this automatically via the CMS (in a separate ticket) than here (and elsewhere
> in content).
> 
> I vote we remove the `rel`s here (which pose no real threat or significant
> performance benefit) and create a separate Sitescripts issue to address the
> potential threats and benefits globally elsewhere.

I agree. It definitely makes sense to have this automated. I will remove the
attributes from this case.

https://codereview.adblockplus.org/29490624/diff/29490625/includes/sidebar/pr...
includes/sidebar/primary-navigation.tmpl:23: <a href="{{ sitemap_page }}"
target="_blank" rel="noopener noreferrer">
On 2017/07/19 15:03:31, juliandoucette wrote:
> On 2017/07/19 12:55:22, ire wrote:
> > From what I can tell it seems like a good idea to, as a default, apply this
to
> > external links. In this case, perhaps the noreferrer value should be
excluded
> > since we are linking to our own blog and may want the stats. Regarding the
> > nofollow value, you're probably right that we can trust http://medium.com
not
> to attack
> > us. However, there may still be some performance benefits to be gained from
> > using it. What do you think? Is it worth including at all?
> 
> - I want to exclude `noreferrer` in this case
> - I want to keep `noopener` in general
> 
> By this logic, we should add both of these `rel`s to `a`s by default and
exclude
> `noreferrer` if the `href` is whitelisted. But I think it's a better idea to
do
> this automatically via the CMS (in a separate ticket) than here (and elsewhere
> in content).
> 
> I vote we remove the `rel`s here (which pose no real threat or significant
> performance benefit) and create a separate Sitescripts issue to address the
> potential threats and benefits globally elsewhere.

Done.

juliandoucette

LGTM + NITs It seems the AAC blog posts have been taken offline temporarily. Please ...

July 19, 2017, 7:59 p.m. (2017-07-19 19:59:46 UTC) #7

ire

July 20, 2017, 1:03 p.m. (2017-07-20 13:03:56 UTC) #8

I've addressed the NITs. Will wait for the go-ahead before I push.

https://codereview.adblockplus.org/29490624/diff/29492626/includes/sidebar/pr...
File includes/sidebar/primary-navigation.tmpl (right):

https://codereview.adblockplus.org/29490624/diff/29492626/includes/sidebar/pr...
includes/sidebar/primary-navigation.tmpl:25: <img
src="/img/png/icon-external-link.png" srcset="/img/png/icon-external-link.png,
/img/svg/icon-external-link.svg 2x" alt="{{ "External Link" |
translate("external-link-icon", "Image alt text") }}" class="external-icon">
On 2017/07/19 19:59:45, juliandoucette wrote:
> NIT: I think this class name is too specific. These styles are also
appropriate
> for other navigation icons at least.

Done.

https://codereview.adblockplus.org/29490624/diff/29492626/includes/sidebar/pr...
includes/sidebar/primary-navigation.tmpl:25: <img
src="/img/png/icon-external-link.png" srcset="/img/png/icon-external-link.png,
/img/svg/icon-external-link.svg 2x" alt="{{ "External Link" |
translate("external-link-icon", "Image alt text") }}" class="external-icon">
On 2017/07/19 19:59:45, juliandoucette wrote:
> NIT: I don't think we need to the 1x src in srcset.

Done.

https://codereview.adblockplus.org/29490624/diff/29492626/static/scss/layout/...
File static/scss/layout/_sidebar.scss (right):

https://codereview.adblockplus.org/29490624/diff/29492626/static/scss/layout/...
static/scss/layout/_sidebar.scss:267: margin-left: $xs;
On 2017/07/19 19:59:45, juliandoucette wrote:
> NIT: $xs / 2 looks better to me.
> 
> (Trust your best judgement and/or Jeen's order.)

Acknowledged. Asked Jeen and she prefers the $xs size so sticking with it.

Expand All Messages | Collapse All Messages