Issue 29333092: Issue 3438 - Added <link rel=canonical> tag to adblockbrowser.org

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 29333092: Issue 3438 - Added <link rel=canonical> tag to adblockbrowser.org (Closed)

Created:
Dec. 28, 2015, 2:49 p.m. by Sebastian Noack

Modified:
Dec. 28, 2015, 5:49 p.m.

Reviewers:
saroyanm

Visibility:
Public.

Description

Issue 3438 - Added <link rel=canonical> tag to adblockbrowser.org

Patch Set 2 : Indicate empty tag #

Created: Dec. 28, 2015, 4:37 p.m.

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1 line, -0 lines)			Patch
	M	templates/default.tmpl	View	1	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

saroyanm

LGTM with small nit. https://codereview.adblockplus.org/29333092/diff/29333095/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29333092/diff/29333095/templates/default.tmpl#newcode25 templates/default.tmpl:25: <link rel="canonical" href="https://adblockbrowser.org/{{ page[:-5] if ...

Dec. 28, 2015, 3:35 p.m. (2015-12-28 15:35:05 UTC) #2

Sebastian Noack

https://codereview.adblockplus.org/29333092/diff/29333095/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29333092/diff/29333095/templates/default.tmpl#newcode25 templates/default.tmpl:25: <link rel="canonical" href="https://adblockbrowser.org/{{ page[:-5] if page.split('/')[-1] == 'index' else ...

Dec. 28, 2015, 4:38 p.m. (2015-12-28 16:38:03 UTC) #3

saroyanm

Dec. 28, 2015, 5:08 p.m. (2015-12-28 17:08:38 UTC) #4

On 2015/12/28 16:38:03, Sebastian Noack wrote:
>
https://codereview.adblockplus.org/29333092/diff/29333095/templates/default.tmpl
> File templates/default.tmpl (right):
> 
>
https://codereview.adblockplus.org/29333092/diff/29333095/templates/default.t...
> templates/default.tmpl:25: <link rel="canonical"
> href="https://adblockbrowser.org/{{ page[:-5] if page.split('/')[-1] ==
'index'
> else page }}">
> On 2015/12/28 15:35:05, saroyanm wrote:
> > Nit: Please also close the tag, to make it consistent with other
void-elements
> > in the template.
> 
> Well, I'm almost certain that this wrong in the context of HTML5. But done,
for
> consistency.

It's not wrong, but it doesn't have effect:
http://dev.w3.org/html5/spec-author-view/syntax.html#syntax-start-tag 
"if the element is one of the void elements, or if the element is a foreign
element, then there may be a single U+002F SOLIDUS character (/)."
But having closer look to our coding standards that are inherited from Google we
shouldn't use it, anyway let's make it consistent in current context, this
template was created before we introduced HTML/CSS coding styles.
https://google.github.io/styleguide/htmlcssguide.xml?showone=Document_Type#Do...

LGTM

Expand All Messages | Collapse All Messages