Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(29)

Issue 29844604: Fixes #115 - Added mobile section to specific criteria of acceptable ads page

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by juliandoucette
Modified:
1 week, 3 days ago
Reviewers:
ire, l.ursachi, wspee
CC:
saroyanm
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

Fixes #115 - Added mobile section to specific criteria of acceptable ads page

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -0 lines) Patch
M pages/acceptable-ads.md View 2 chunks +26 lines, -0 lines 2 comments Download

Messages

Total messages: 3
juliandoucette
1 week, 5 days ago (2018-08-01 15:02:42 UTC) #1
l.ursachi
Added a quick and dirty code review - not sure if enough, though :-) https://codereview.adblockplus.org/29844604/diff/29844605/pages/acceptable-ads.md ...
1 week, 3 days ago (2018-08-03 11:31:43 UTC) #2
wspee
1 week, 3 days ago (2018-08-03 13:40:32 UTC) #3
On 2018/08/03 11:31:43, l.ursachi wrote:
> Added a quick and dirty code review - not sure if enough, though :-)
> 
>
https://codereview.adblockplus.org/29844604/diff/29844605/pages/acceptable-ad...
> File pages/acceptable-ads.md (right):
> 
>
https://codereview.adblockplus.org/29844604/diff/29844605/pages/acceptable-ad...
> pages/acceptable-ads.md:94: #### {{ mobile-ads-heading[heading] MOBILE ADS }}
> Could we please have ```MOBILE ADS``` in sentence case in order to be
consistent
> with the other H4s? I know it was in upper case in the copy doc but only saw
it
> now :D
> 
>
https://codereview.adblockplus.org/29844604/diff/29844605/pages/acceptable-ad...
> pages/acceptable-ads.md:98: - {{ mobile-ads-placement-item-1[list item] Static
> ad types (e.g. 6x1 banner and 1x1 tile ad) are allowed to be placed anywhere
on
> the mobile page }}
> Add ```.``` at the end of the sentence

FWIW I didn't find anything else, the rest of the changes work and look fine.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5