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

Issue 29749588: Fixes #53 - Added detection and install links for iOS safari (Closed)

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Addressed #2 #

Patch Set 3 : Separated samsung browser #

Total comments: 7

Patch Set 4 : Addressed #6 #

Patch Set 5 : Addressed #8 #

Total comments: 3

Patch Set 6 : Addressed #13 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -1 line) Patch
M static/css/index.css View 1 2 3 4 5 1 chunk +2 lines, -1 line 1 comment Download

Messages

Total messages: 18
juliandoucette
5 months, 1 week ago (2018-04-11 20:48:25 UTC) #1
ire
Thanks Julian! A couple comments/questions: 1. You didn't handle this: > The "Install for Safari" ...
5 months, 1 week ago (2018-04-12 07:44:48 UTC) #2
juliandoucette
Thanks Ire! New Patchset up :) > The "Install for Safari" should probably be renamed ...
5 months, 1 week ago (2018-04-12 11:58:01 UTC) #3
ire
On 2018/04/12 11:58:01, juliandoucette wrote: > > The "Install for Safari" should probably be renamed ...
5 months, 1 week ago (2018-04-12 14:30:32 UTC) #4
juliandoucette
> Okay. Can you please modify the issue description then? And let Tiago know as ...
5 months, 1 week ago (2018-04-12 15:02:24 UTC) #5
ire
Thanks Julian! Responses: https://codereview.adblockplus.org/29749588/diff/29750569/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29749588/diff/29750569/includes/index.tmpl#newcode78 includes/index.tmpl:78: <a class="install-button" href="https://itunes.apple.com/us/app/adblock-plus-abp-remove-ads-browse-faster-without-tracking/id1028871868?mt=8" id="install-ios">{{"Agree and Install ...
5 months, 1 week ago (2018-04-12 15:13:50 UTC) #6
juliandoucette
New Patchset up! https://codereview.adblockplus.org/29749588/diff/29750569/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29749588/diff/29750569/static/css/index.css#newcode246 static/css/index.css:246: #content.safari .install-button, On 2018/04/12 15:13:50, ire ...
5 months, 1 week ago (2018-04-12 15:30:39 UTC) #7
juliandoucette
(Wait! One more issue to fix...)
5 months, 1 week ago (2018-04-12 15:32:50 UTC) #8
juliandoucette
On 2018/04/12 15:32:50, juliandoucette wrote: > (Wait! One more issue to fix...) Done.
5 months, 1 week ago (2018-04-12 15:33:49 UTC) #9
ire
Thanks Julian! Just one more thing https://codereview.adblockplus.org/29749588/diff/29750618/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29749588/diff/29750618/static/css/index.css#newcode320 static/css/index.css:320: #content.safari.ua-safari #install-safari, You ...
5 months, 1 week ago (2018-04-13 08:41:14 UTC) #10
juliandoucette
https://codereview.adblockplus.org/29749588/diff/29750618/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29749588/diff/29750618/static/css/index.css#newcode320 static/css/index.css:320: #content.safari.ua-safari #install-safari, On 2018/04/13 08:41:14, ire wrote: > You ...
5 months, 1 week ago (2018-04-13 11:58:22 UTC) #11
juliandoucette
Done https://hg.adblockplus.org/web.adblockplus.org/rev/270c61b6ff95.
5 months ago (2018-04-16 12:20:38 UTC) #12
ire
Hey Julian, I still have a comment (below). The Install button doesn't show on non-Safari ...
5 months ago (2018-04-16 15:14:42 UTC) #13
juliandoucette
On 2018/04/16 15:14:42, ire wrote: > Yes, but now neither button will show then the ...
5 months ago (2018-04-16 15:18:55 UTC) #14
juliandoucette
New Patchset up. The new patchset applies to the latest revision of master (since I ...
5 months ago (2018-04-16 15:26:41 UTC) #15
ire
On 2018/04/16 15:26:41, juliandoucette wrote: > New Patchset up. > > The new patchset applies ...
5 months ago (2018-04-16 15:42:21 UTC) #16
juliandoucette
On 2018/04/16 15:42:21, ire wrote: > https://codereview.adblockplus.org/29749588/diff/29753590/static/css/index.css#newcode327 > static/css/index.css:327: #content.safari:not(.ua-ios) #install-safari { > display: inline-block; ...
5 months ago (2018-04-16 15:47:45 UTC) #17
juliandoucette
5 months ago (2018-04-16 15:56:52 UTC) #18
On 2018/04/16 15:47:45, juliandoucette wrote:
> I will keep it this way and add a comment about this exception (that this rule
is
> necessary because their are two safaris) for now.

Scratch that. I'll fix your NIT.
Sign in to reply to this message.

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