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

Issue 29766571: Fixes #55 - Updated install button on Samsung Browser for Android

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 6 days ago by juliandoucette
Modified:
5 days, 15 hours ago
Reviewers:
ire
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/55#note_70975416.

Patch Set 1 : First draft #

Patch Set 2 : Ready for review #

Total comments: 7

Patch Set 3 : Addressed #3 #

Total comments: 4

Patch Set 4 : Addressed #5 #

Patch Set 5 : Addressed #6 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+673 lines, -0 lines) Patch
M pages/android.tmpl View 1 2 3 4 1 chunk +53 lines, -0 lines 2 comments Download
A static/js/vendor/bowser.js View 1 chunk +620 lines, -0 lines 0 comments Download

Messages

Total messages: 10
juliandoucette
2 weeks, 6 days ago (2018-04-30 21:10:16 UTC) #1
juliandoucette
Ready for review. See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/55#note_70975416 first.
2 weeks, 6 days ago (2018-04-30 21:56:54 UTC) #2
ire
Thanks Julian! On 2018/04/30 21:56:54, juliandoucette wrote: > Ready for review. > > See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/55#note_70975416 ...
2 weeks, 5 days ago (2018-05-01 08:04:26 UTC) #3
juliandoucette
Thanks Ire! Ready for review again. https://codereview.adblockplus.org/29766571/diff/29766578/pages/android.tmpl File pages/android.tmpl (right): https://codereview.adblockplus.org/29766571/diff/29766578/pages/android.tmpl#newcode3 pages/android.tmpl:3: /* Hide TOS, ...
2 weeks, 5 days ago (2018-05-01 11:14:36 UTC) #4
juliandoucette
A few helpful (I hope) notes. https://codereview.adblockplus.org/29766571/diff/29767565/pages/android.tmpl File pages/android.tmpl (right): https://codereview.adblockplus.org/29766571/diff/29767565/pages/android.tmpl#newcode4 pages/android.tmpl:4: .android:not(.ua-samsung) #terms-message, Detail: ...
2 weeks, 5 days ago (2018-05-01 11:21:56 UTC) #5
juliandoucette
One more mistake *facepalm*... I've re-displayed the Adblock Browser notice in Samsung Browser in my ...
2 weeks, 5 days ago (2018-05-01 11:32:36 UTC) #6
ire
Thanks Julian! Assuming Luiza is fine with your changes then this implementation looks good (+ ...
2 weeks, 4 days ago (2018-05-02 08:07:00 UTC) #7
juliandoucette
(Been chatting with Liz on IRC. There are more changes coming to the ticket. I ...
2 weeks, 3 days ago (2018-05-03 13:56:03 UTC) #8
juliandoucette
Liz discussed it with Tiago and got back to me. The conclusions were: 1. The ...
5 days, 21 hours ago (2018-05-15 10:08:43 UTC) #9
ire
5 days, 15 hours ago (2018-05-15 15:14:47 UTC) #10
On 2018/05/15 10:08:43, juliandoucette wrote:
> Liz discussed it with Tiago and got back to me. The conclusions were:
> 
> 1. The install buttons should install ABP for mobile not ABB mobile
> 2. We should remove the ABB banner on http://abp.org and show the ABB
> alert/notification below the ABP install button like we do on desktop
> 
> These changes fall awkwardly close to the launch of the new home page (which
> contradicts them) which we have just received translations for.
> 
> I think we should address these changes in two separate tickets per browser (1
> of which - iOS safari install button - is already completed).

Alright. IMO if we are planning to launch the home page within the next 1-2
weeks, we may as well not publish this change.
Sign in to reply to this message.

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