Issue 29766571: Fixes #55 - Updated install button on Samsung Browser for Android (Closed)
|
Created:
April 30, 2018, 9:10 p.m. by juliandoucette Modified:
May 25, 2018, 8:51 a.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
DescriptionSee 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
MessagesTotal messages: 11
Ready for review. See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/55#note_70975416 first.
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 > first. Thanks! 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#... pages/android.tmpl:3: /* Hide TOS, install button, and platform buttons on not-samsung Android */ Although I agree that these elements may not be very useful on Android, I don't think we should make this change without getting input on it's potential impact. Since it's not directly related to this particular issue, I would prefer you handle it separately. Also it seems Luiza mentioned the banner should remain? https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/55#note_70890051 https://codereview.adblockplus.org/29766571/diff/29766578/pages/android.tmpl#... pages/android.tmpl:22: .classList.remove("ua-android"); Suggest: Add "ua-samsung" to the #content and hide/show the appropriate buttons etc through CSS instead https://codereview.adblockplus.org/29766571/diff/29766578/pages/android.tmpl#... pages/android.tmpl:33: .textContent = "Agree and Install for Samsung Browser"; This text should be "Agree and Install for Samsung Internet"
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#... pages/android.tmpl:3: /* Hide TOS, install button, and platform buttons on not-samsung Android */ On 2018/05/01 08:04:25, ire wrote: > Although I agree that these elements may not be very useful on Android, I don't > think we should make this change without getting input on it's potential impact. > Since it's not directly related to this particular issue, I would prefer you > handle it separately. > > Also it seems Luiza mentioned the banner should remain? > https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/55#note_70890051 RE: Changing this without Product feedback I agree. I should have made it clearer that I won't move forward without product feedback. Sorry about that. And I agree about handling this separately. But I'd prefer to handle it in two commits to one issue if you don't mind (since it's already part of the issue on gitlab). RE: Luiza banner or not Luiza changed her mind from option 1 to option 2 of those originally presented in my description. After that, I updated my description accordingly (no banner IIRC). https://codereview.adblockplus.org/29766571/diff/29766578/pages/android.tmpl#... pages/android.tmpl:22: .classList.remove("ua-android"); On 2018/05/01 08:04:25, ire wrote: > Suggest: Add "ua-samsung" to the #content and hide/show the appropriate buttons > etc through CSS instead Good idea. That will make testing easier. https://codereview.adblockplus.org/29766571/diff/29766578/pages/android.tmpl#... pages/android.tmpl:33: .textContent = "Agree and Install for Samsung Browser"; On 2018/05/01 08:04:25, ire wrote: > This text should be "Agree and Install for Samsung Internet" Good catch.
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#... pages/android.tmpl:4: .android:not(.ua-samsung) #terms-message, Detail: `.android` is applied to the android page by default.tmpl (it flags the page, not the browser). https://codereview.adblockplus.org/29766571/diff/29767565/pages/android.tmpl#... pages/android.tmpl:7: .ua-andriod.android #other-platform, Detail: ua-* classes are injected by the server (with the one exception of samsung below). Therefore, this applies to the android page when using android only. https://codereview.adblockplus.org/29766571/diff/29767565/pages/android.tmpl#... pages/android.tmpl:9: #content.ua-android.ua-samsung #abb-banner Detail: `.ua-android` is injected by the server. `.ua-samsung` is added by the script below. I decided to keep and use them both based on your suggestion and the fact that both are true e.g. `bowser.android` and `bowser.samsungBrowser` are both true at once. This is also convenient because it allows me to make a more specific selector than `#content.ua-android #abb-banner` that already exists to display this banner block. https://codereview.adblockplus.org/29766571/diff/29767565/pages/android.tmpl#... pages/android.tmpl:31: document Oops... I'll use the content variable above in the next PatchSet.
One more mistake *facepalm*... I've re-displayed the Adblock Browser notice in Samsung Browser in my last patchset like I said I did on Gitlab.
Thanks Julian! Assuming Luiza is fine with your changes then this implementation looks good (+ couple comments below) 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#... pages/android.tmpl:3: /* Hide TOS, install button, and platform buttons on not-samsung Android */ On 2018/05/01 11:14:36, juliandoucette wrote: > On 2018/05/01 08:04:25, ire wrote: > > Although I agree that these elements may not be very useful on Android, I > don't > > think we should make this change without getting input on it's potential > impact. > > Since it's not directly related to this particular issue, I would prefer you > > handle it separately. > > > > Also it seems Luiza mentioned the banner should remain? > > https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/55#note_70890051 > > RE: Changing this without Product feedback > > I agree. I should have made it clearer that I won't move forward without product > feedback. Sorry about that. And I agree about handling this separately. But I'd > prefer to handle it in two commits to one issue if you don't mind (since it's > already part of the issue on gitlab). Ack, that is fine. > RE: Luiza banner or not > > Luiza changed her mind from option 1 to option 2 of those originally presented > in my description. After that, I updated my description accordingly (no banner > IIRC). In the comment I linked to above she wrote "Please let's keep the banner there for now." . Maybe confirm form her again so we are sure? https://codereview.adblockplus.org/29766571/diff/29767574/pages/android.tmpl File pages/android.tmpl (right): https://codereview.adblockplus.org/29766571/diff/29767574/pages/android.tmpl#... pages/android.tmpl:7: .ua-andriod.android #other-platform, Typo in "ua-android" https://codereview.adblockplus.org/29766571/diff/29767574/pages/android.tmpl#... pages/android.tmpl:28: var contentElement = document.getElementById("content"); Coding style -> "No hungarian notation"
(Been chatting with Liz on IRC. There are more changes coming to the ticket. I will update you when it's been finalized.)
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 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).
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.
Closing for now. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
