pages/android.tmpl - Issue 29766571: Fixes #55 - Updated install button on Samsung Browser for Android

Delta Between Two Patch Sets: pages/android.tmpl

Issue 29766571: Fixes #55 - Updated install button on Samsung Browser for Android (Closed) Base URL: https://hg.adblockplus.org/web.adblockplus.org

Left Patch Set: Ready for review Created April 30, 2018, 9:53 p.m.

Right Patch Set: Addressed #6 Created May 1, 2018, 11:34 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 <head>	1 <head>

2 <style>	2 <style>

3 /* Hide TOS, install button, and platform buttons on not-samsung Android */	3 /* Hide TOS & Install on android page when not using Samsung Browser */
ire 2018/05/01 08:04:25 Although I agree that these elements may not be ve 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 juliandoucette 2018/05/01 11:14:36 RE: Changing this without Product feedback I agre Show quoted text 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). ire 2018/05/02 08:06:59 Ack, that is fine. Show quoted text 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. Show quoted text > 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?
4 .ua-android #terms-message,	4 .android:not(.ua-samsung) #terms-message,

5 .ua-android #install,	5 .android:not(.ua-samsung) #install,

6 .android #other-platform	6 /* Hide other platforms on android page when using Android */

	7 .ua-andriod.android #other-platform,
	ire 2018/05/02 08:06:59 Typo in "ua-android" Typo in "ua-android"
	8 /* Hide ABB banner on android page when using Samsung Browser */

	9 #content.ua-android.ua-samsung #abb-banner

7 {	10 {

8 display: none;	11 display: none;

	12 }

	13

	14 /* Show ABB notification under install button when using Samsung Browser */

	15 #content.ua-android.ua-samsung #adblock-browser-notification

	16 {

	17 display: block;

9 }	18 }

10 </style>	19 </style>

11 <script src="/js/vendor/bowser.js"></script>	20 <script src="/js/vendor/bowser.js"></script>

12 <script>	21 <script>

13 document.addEventListener("DOMContentLoaded", function()	22 document.addEventListener("DOMContentLoaded", function()

14 {	23 {

	24 if (!bowser instanceof Object) return;

	25

15 var locale = "{{ locale }}";	26 var locale = "{{ locale }}";

16	27

17 if (bowser instanceof Object && bowser.samsungBrowser)	28 var contentElement = document.getElementById("content");
	ire 2018/05/02 08:06:59 Coding style -> "No hungarian notation" Coding style -> "No hungarian notation"
	29

	30 // Replicating server behaviour on the frontend for testing

	31 if (bowser.android)

	32 contentElement.classList.add("ua-android");

	33

	34 if (bowser.samsungBrowser)

18 {	35 {

19 // hide the Adblock Browser banner	36 // Assign more specific user agent (ua-android is already present)

20 document	37 contentElement.classList.add("ua-samsung");

21 .getElementById("content")

22 .classList.remove("ua-android");
ire 2018/05/01 08:04:25 Suggest: Add "ua-samsung" to the #content and hide Suggest: Add "ua-samsung" to the #content and hide/show the appropriate buttons etc through CSS instead juliandoucette 2018/05/01 11:14:36 Good idea. That will make testing easier. Show quoted text 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.
23	38

24 // change the Android install link to Samsung Browser	39 // change the Android install link to Samsung Browser

25 document	40 document

26 .getElementById("install-android")	41 .getElementById("install-android")

27 .href = "https://play.google.com/store/apps/details?id=org.adblockplus .adblockplussbrowser";	42 .href = "https://play.google.com/store/apps/details?id=org.adblockplus .adblockplussbrowser";

28	43

29 // update the install button text in English only	44 // update the install button text in English only

30 if (locale == "en")	45 if (locale == "en")

31 document	46 document

32 .getElementById("install-android")	47 .getElementById("install-android")

33 .textContent = "Agree and Install for Samsung Browser";	48 .textContent = "Agree and Install for Samsung Internet";
ire 2018/05/01 08:04:25 This text should be "Agree and Install for Samsung This text should be "Agree and Install for Samsung Internet" juliandoucette 2018/05/01 11:14:36 Good catch. Show quoted text On 2018/05/01 08:04:25, ire wrote: > This text should be "Agree and Install for Samsung Internet" Good catch.
34 }	49 }

35 });	50 });

36 </script>	51 </script>

37 </head>	52 </head>

38	53

39 <? include index ?>	54 <? include index ?>

LEFT	RIGHT