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

Issue 5440736082264064: Issue 2075 - Put meta tags inside of page header. (Closed)

Created:
March 8, 2015, 2:40 p.m. by kzar
Modified:
March 9, 2015, 1:08 p.m.
Visibility:
Public.

Description

Issue 2075 - Put meta tags inside of page header.

Patch Set 1 #

Patch Set 2 : Changes generated by fixed conversion script. #

Total comments: 1

Patch Set 3 : Wrap meta tags in head even when head doesn't yet exist. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -12 lines) Patch
M includes/index.tmpl View 1 2 1 chunk +1 line, -1 line 0 comments Download
M pages/acceptable-ads.raw View 1 2 1 chunk +2 lines, -2 lines 1 comment Download
M pages/acceptable-ads-manifesto.raw View 1 2 1 chunk +1 line, -1 line 0 comments Download
M pages/bugs.raw View 1 2 1 chunk +2 lines, -2 lines 1 comment Download
M pages/contribute.raw View 1 2 1 chunk +1 line, -1 line 0 comments Download
M pages/features.raw View 1 2 1 chunk +1 line, -1 line 0 comments Download
M pages/getting_started.raw View 1 2 1 chunk +2 lines, -2 lines 1 comment Download
M pages/tutorials.raw View 1 2 1 chunk +2 lines, -2 lines 1 comment Download

Messages

Total messages: 13
kzar
March 8, 2015, 2:42 p.m. (2015-03-08 14:42:41 UTC) #1
Wladimir Palant
This should be done by the conversion script, not manually - we will be running ...
March 8, 2015, 7:14 p.m. (2015-03-08 19:14:30 UTC) #2
kzar
Fair enough, I'll try and do that when I get a chance.
March 8, 2015, 7:28 p.m. (2015-03-08 19:28:13 UTC) #3
Wladimir Palant
For reference, the PHP code currently doing that is the following: $regexp = '/<meta\b[^>]*>\s*/is'; if ...
March 8, 2015, 7:33 p.m. (2015-03-08 19:33:10 UTC) #4
kzar
Patch Set 2 : Changes generated by fixed conversion script. Thanks, I updated the conversion ...
March 8, 2015, 9:54 p.m. (2015-03-08 21:54:34 UTC) #5
kzar
Sorry for spam, just realised that last commit to conversion script was a little ugly. ...
March 8, 2015, 10:14 p.m. (2015-03-08 22:14:28 UTC) #6
Wladimir Palant
LGTM Note that I modified the regexp slightly to make sure the resulting whitespace looks ...
March 8, 2015, 10:24 p.m. (2015-03-08 22:24:05 UTC) #7
saroyanm
We have some pages whose header looks like this: <head><anwv/></head> So this line in your ...
March 9, 2015, 11:21 a.m. (2015-03-09 11:21:33 UTC) #8
kzar
Patch Set 3 : Wrap meta tags in head even when head doesn't yet exist. ...
March 9, 2015, 11:49 a.m. (2015-03-09 11:49:57 UTC) #9
Wladimir Palant
My bad, didn't know that we have metatags in some more pages. Frankly, I wonder ...
March 9, 2015, 11:55 a.m. (2015-03-09 11:55:50 UTC) #10
saroyanm
LGTM with fixed nits. http://codereview.adblockplus.org/5440736082264064/diff/5766466041282560/pages/acceptable-ads.raw File pages/acceptable-ads.raw (right): http://codereview.adblockplus.org/5440736082264064/diff/5766466041282560/pages/acceptable-ads.raw#newcode3 pages/acceptable-ads.raw:3: <head><meta content="$s1$" name="title"> nit: Please ...
March 9, 2015, 12:51 p.m. (2015-03-09 12:51:47 UTC) #11
Wladimir Palant
Manvel, this is autogenerated - the style isn't always great, this is "normal".
March 9, 2015, 12:59 p.m. (2015-03-09 12:59:09 UTC) #12
saroyanm
March 9, 2015, 1:08 p.m. (2015-03-09 13:08:17 UTC) #13 
On 2015/03/09 12:59:09, Wladimir Palant wrote:
> Manvel, this is autogenerated - the style isn't always great, this is
"normal".

LGTM in that case

Powered by Google App Engine
This is Rietveld