Issue 29749588: Fixes #53 - Added detection and install links for iOS safari (Closed)
|
Created:
April 11, 2018, 8:48 p.m. by juliandoucette Modified:
April 17, 2018, 10:32 a.m. 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
MessagesTotal messages: 18
Thanks Julian! A couple comments/questions: 1. You didn't handle this: > The "Install for Safari" should probably be renamed to "Install for Safari iOS" 2. The issue seems to be only about iOS, not Samsung Browser? https://codereview.adblockplus.org/29749588/diff/29749589/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29749588/diff/29749589/includes/index.tmpl... includes/index.tmpl:202: var installButton = document.querySelector("install-button"); Missing class identifier ".install-button" Also, this is always selecting the firefox-button. It's still in the document even though it has `display: none` Suggest: Select the appropriate button within the if statements https://codereview.adblockplus.org/29749588/diff/29749589/includes/index.tmpl... includes/index.tmpl:208: installButton.setAttribute("href", "https://itunes.apple.com/us/app/adblock-plus-abp-remove-ads-browse-faster-without-tracking/id1028871868?mt=8"); Why are you handling this here instead of the way the other install buttons are (i.e. in the markup instead of dynamically changing the link)? This would mean that, no matter what browser page the user is on, the link would be to the ios/samsung version if they are in one of those browsers.
Thanks Ire! New Patchset up :) > The "Install for Safari" should probably be renamed to "Install for Safari iOS" I don't think this is necessary because [safari is the name of the browser that we are targeting on ios, we would have to translate it (and we may replace this entire component before we do that)]. 2. The issue seems to be only about iOS, not Samsung Browser? See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/53#note_68031300 On second thought, I'll separate Safari iOS and Samsung into separate issues because Samsung browser is not as straightforward. https://codereview.adblockplus.org/29749588/diff/29749589/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29749588/diff/29749589/includes/index.tmpl... includes/index.tmpl:202: var installButton = document.querySelector("install-button"); On 2018/04/12 07:44:48, ire wrote: > Missing class identifier ".install-button" Wow... *blushes* > Also, this is always selecting the firefox-button. It's still in the document > even though it has `display: none` > > Suggest: Select the appropriate button within the if statements Good catch. https://codereview.adblockplus.org/29749588/diff/29749589/includes/index.tmpl... includes/index.tmpl:208: installButton.setAttribute("href", "https://itunes.apple.com/us/app/adblock-plus-abp-remove-ads-browse-faster-without-tracking/id1028871868?mt=8"); On 2018/04/12 07:44:48, ire wrote: > Why are you handling this here instead of the way the other install buttons are > (i.e. in the markup instead of dynamically changing the link)? I ~incorrectly assumed that our backend browser detection would not handle this properly. > This would mean that, no matter what browser page the user is on, the link would > be to the ios/samsung version if they are in one of those browsers. Good catch.
On 2018/04/12 11:58:01, juliandoucette wrote: > > The "Install for Safari" should probably be renamed to "Install for Safari > iOS" > > I don't think this is necessary because [safari is the name of the browser that > we are targeting on ios, we would have to translate it (and we may replace this > entire component before we do that)]. Okay. Can you please modify the issue description then? And let Tiago know as I believe he's the one that wrote it that way. > 2. The issue seems to be only about iOS, not Samsung Browser? > > See https://gitlab.com/eyeo/websites/web.adblockplus.org/issues/53#note_68031300 > > On second thought, I'll separate Safari iOS and Samsung into separate issues > because Samsung browser is not as straightforward. Thanks! 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... 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 for Safari"|translate("s18")}}</a> Suggestion/Question: Since this string is identical to the one above, is there a way to output the same string by referencing the ID without repeating the string itself? 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.cs... static/css/index.css:246: #content.safari .install-button, What's the reason for these changes? (where you remove the .ua-safari additional class)
> Okay. Can you please modify the issue description then? And let Tiago know as I believe he's the one that wrote it that way. Already done :) 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... 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 for Safari"|translate("s18")}}</a> On 2018/04/12 14:30:32, ire wrote: > Suggestion/Question: Since this string is identical to the one above, is there a > way to output the same string by referencing the ID without repeating the string > itself? ~Yes. But it's not pretty in .tmpl format. It looks like {{ "" | translate("s18") }}. I'd prefer to leave it as-is for now. 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.cs... static/css/index.css:246: #content.safari .install-button, On 2018/04/12 14:30:32, ire wrote: > What's the reason for these changes? (where you remove the .ua-safari additional > class) I think that #content.safari applies to #content.safari.ua-safari and #content.safari.ua-ios appropriately (we can use one selector instead of two) for these use cases.
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... 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 for Safari"|translate("s18")}}</a> On 2018/04/12 15:02:23, juliandoucette wrote: > On 2018/04/12 14:30:32, ire wrote: > > Suggestion/Question: Since this string is identical to the one above, is there > a > > way to output the same string by referencing the ID without repeating the > string > > itself? > > ~Yes. But it's not pretty in .tmpl format. It looks like {{ "" | > translate("s18") }}. > > I'd prefer to leave it as-is for now. Ah okay, that's fine. 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.cs... static/css/index.css:246: #content.safari .install-button, On 2018/04/12 15:02:23, juliandoucette wrote: > On 2018/04/12 14:30:32, ire wrote: > > What's the reason for these changes? (where you remove the .ua-safari > additional > > class) > > I think that #content.safari applies to #content.safari.ua-safari and > #content.safari.ua-ios appropriately (we can use one selector instead of two) > for these use cases. But these styles should only be applied if the both .safari and .ua-safari are present. This is to check if the user is visiting the /safari page in a Safari browser. If you remove that qualifier then the button will look the same no matter what browser the user is visiting the page on.
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.cs... static/css/index.css:246: #content.safari .install-button, On 2018/04/12 15:13:50, ire wrote: > But these styles should only be applied if the both .safari and .ua-safari are > present. This is to check if the user is visiting the /safari page in a Safari > browser. If you remove that qualifier then the button will look the same no > matter what browser the user is visiting the page on. Oops! (I had things backwards.)
(Wait! One more issue to fix...)
On 2018/04/12 15:32:50, juliandoucette wrote: > (Wait! One more issue to fix...) Done.
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.cs... static/css/index.css:320: #content.safari.ua-safari #install-safari, You don't need the .ua-safari or .ua-ios here because this style is just displaying the button itself.
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.cs... static/css/index.css:320: #content.safari.ua-safari #install-safari, On 2018/04/13 08:41:14, ire wrote: > You don't need the .ua-safari or .ua-ios here because this style is just > displaying the button itself. There are two buttons. One for ua-safari, and another for ua-ios. That's why I can't *just* use #content.safari (two would display instead of one). Context: The browser name class without "ua" comes from the page name.
Message was sent while issue was closed.
Hey Julian, I still have a comment (below). The Install button doesn't show on non-Safari browsers with this change. 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.cs... static/css/index.css:320: #content.safari.ua-safari #install-safari, On 2018/04/13 11:58:22, juliandoucette wrote: > On 2018/04/13 08:41:14, ire wrote: > > You don't need the .ua-safari or .ua-ios here because this style is just > > displaying the button itself. > > There are two buttons. One for ua-safari, and another for ua-ios. That's why I > can't *just* use #content.safari (two would display instead of one). > > Context: The browser name class without "ua" comes from the page name. Yes, but now neither button will show then the user visits the /safari page in a browser that isn't Safari or iOS See https://adblockplus.org/en/safari
On 2018/04/16 15:14:42, ire wrote: > Yes, but now neither button will show then the user visits the /safari page in a > browser that isn't Safari or iOS > > See https://adblockplus.org/en/safari Good point :S I'm on it.
New Patchset up. The new patchset applies to the latest revision of master (since I already pushed the last change pre-maturely). I think that this resolves the issue that you discovered. I'm ok with this ~quick-fix until we publish the new index page install button. Otherwise, we/I may have to backout and re-think how to implement this.
On 2018/04/16 15:26:41, juliandoucette wrote: > New Patchset up. > > The new patchset applies to the latest revision of master (since I already > pushed the last change pre-maturely). > > I think that this resolves the issue that you discovered. I'm ok with this > ~quick-fix until we publish the new index page install button. Otherwise, we/I > may have to backout and re-think how to implement this. Ack. LGTM + NIT https://codereview.adblockplus.org/29749588/diff/29753590/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29749588/diff/29753590/static/css/index.cs... static/css/index.css:327: #content.safari:not(.ua-ios) #install-safari { display: inline-block; } NIT: You didn't need to make this its own rule, you could just add the selector to the above rule.
On 2018/04/16 15:42:21, ire wrote: > https://codereview.adblockplus.org/29749588/diff/29753590/static/css/index.cs... > static/css/index.css:327: #content.safari:not(.ua-ios) #install-safari { > display: inline-block; } > NIT: You didn't need to make this its own rule, you could just add the selector > to the above rule. Ack. :not is not supported everywhere and I didn't test it across-browsers so I made it it's own rule out of fear of breaking the other. This was bad of me. 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.
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. 
|
