Issue 29341378: Issue 4027 - make the download button available for windows insider users (Closed)
|
Created:
May 13, 2016, 10:19 a.m. by saroyanm Modified:
May 18, 2016, 12:57 p.m. Visibility:
Public. |
DescriptionIssue 4027 - make the download button available for windows insider users
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #
Total comments: 4
Patch Set 3 : Uploaded new image #
Total comments: 2
Patch Set 4 : #
Total comments: 2
MessagesTotal messages: 11
Guys I left some comments in the codereview while it will be I think more easier to align on actual example. https://codereview.adblockplus.org/29341378/diff/29341394/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29341378/diff/29341394/includes/index.tmpl... includes/index.tmpl:72: {{"Adblock Plus is <a href='https://www.microsoft.com/store/apps/adblock-plus/9nblggh4r9nz' target='_blank'>available</a> for Windows Insiders on a build 14342 or higher."|translate("edge-message")}} @Ollie can you please help me updating the message, the initial one you have provided was big so it was overlapping the bullet points for some translations, so will really appreciate if you can help me with finding better wording, or let me know if you are Okey with current one, alternatively I can make font smaller but the message will not be that eye catchy. https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:38: if (!window.navigator.userAgent) userAgent detection through navigator is deprecated ( https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent ), I'm not sure if there is another way to detect the users, but this way is not reliable currently and can start not working at anypoint in the future. Maybe there is a feature that has been added in the new builds or maybe we can check if the extension support is available on Edge versions, please let me know guys if you know better way to detect this.
https://codereview.adblockplus.org/29341378/diff/29341394/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29341378/diff/29341394/includes/index.tmpl... includes/index.tmpl:72: {{"Adblock Plus is <a href='https://www.microsoft.com/store/apps/adblock-plus/9nblggh4r9nz' target='_blank'>available</a> for Windows Insiders on a build 14342 or higher."|translate("edge-message")}} On 2016/05/13 10:50:20, saroyanm wrote: > @Ollie can you please help me updating the message, the initial one you have > provided was big so it was overlapping the bullet points for some translations, > so will really appreciate if you can help me with finding better wording, or let > me know if you are Okey with current one, alternatively I can make font smaller > but the message will not be that eye catchy. I don't think we should link to the Windows Store here, as Adblock Plus won't work for users seeing this message anyway. In fact, if they click the link they might still be able to install it, but it won't work. Also I think it should be "on build 14342" instead of "on a build 14342". https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:32: if(document.getElementById("content").className.indexOf("edge") > -1) Perhaps moving this check inside checkEdgeSupport(). Then you could simply store the value of document.getElementById("content") in a variable instead duplication the lookup. https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:38: if (!window.navigator.userAgent) On 2016/05/13 10:50:20, saroyanm wrote: > userAgent detection through navigator is deprecated ( > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent ), I'm > not sure if there is another way to detect the users, but this way is not > reliable currently and can start not working at anypoint in the future. Maybe > there is a feature that has been added in the new builds or maybe we can check > if the extension support is available on Edge versions, please let me know guys > if you know better way to detect this. I don't think there is a different way to detect the browser versions on the client side. However, I don't expect navigator.userAgent going away any time soon, and in particular not while we need that check here, since a lot of websites rely on it. So I think it's fine to rely on navigator.userAgent here. https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:42: if (match && match.length > 0 && parseFloat(match[1]) >= 14.14342) The check for |match.length > 0| is redundant. If there is a match it will always be an array with one element per group (even if the group would be optional). https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:43: document.getElementById("content").className += " edge-supported"; How about using classList? We don't need to care about compatibility with old IE versions here.
https://codereview.adblockplus.org/29341378/diff/29341394/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29341378/diff/29341394/includes/index.tmpl... includes/index.tmpl:72: {{"Adblock Plus is <a href='https://www.microsoft.com/store/apps/adblock-plus/9nblggh4r9nz' target='_blank'>available</a> for Windows Insiders on a build 14342 or higher."|translate("edge-message")}} On 2016/05/17 08:50:13, Sebastian Noack wrote: > On 2016/05/13 10:50:20, saroyanm wrote: > > @Ollie can you please help me updating the message, the initial one you have > > provided was big so it was overlapping the bullet points for some > translations, > > so will really appreciate if you can help me with finding better wording, or > let > > me know if you are Okey with current one, alternatively I can make font > smaller > > but the message will not be that eye catchy. > > I don't think we should link to the Windows Store here, as Adblock Plus won't > work for users seeing this message anyway. In fact, if they click the link they > might still be able to install it, but it won't work. > > Also I think it should be "on build 14342" instead of "on a build 14342". Done. https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:32: if(document.getElementById("content").className.indexOf("edge") > -1) On 2016/05/17 08:50:13, Sebastian Noack wrote: > Perhaps moving this check inside checkEdgeSupport(). Then you could simply store > the value of document.getElementById("content") in a variable instead > duplication the lookup. Done. https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:38: if (!window.navigator.userAgent) On 2016/05/17 08:50:13, Sebastian Noack wrote: > On 2016/05/13 10:50:20, saroyanm wrote: > > userAgent detection through navigator is deprecated ( > > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent ), I'm > > not sure if there is another way to detect the users, but this way is not > > reliable currently and can start not working at anypoint in the future. Maybe > > there is a feature that has been added in the new builds or maybe we can check > > if the extension support is available on Edge versions, please let me know > guys > > if you know better way to detect this. > > I don't think there is a different way to detect the browser versions on the > client side. However, I don't expect navigator.userAgent going away any time > soon, and in particular not while we need that check here, since a lot of > websites rely on it. So I think it's fine to rely on navigator.userAgent here. Acknowledged. https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:42: if (match && match.length > 0 && parseFloat(match[1]) >= 14.14342) On 2016/05/17 08:50:13, Sebastian Noack wrote: > The check for |match.length > 0| is redundant. If there is a match it will > always be an array with one element per group (even if the group would be > optional). Done. https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:43: document.getElementById("content").className += " edge-supported"; On 2016/05/17 08:50:13, Sebastian Noack wrote: > How about using classList? We don't need to care about compatibility with old IE > versions here. Good point https://codereview.adblockplus.org/29341378/diff/29341394/static/js/index.js#... static/js/index.js:43: document.getElementById("content").className += " edge-supported"; On 2016/05/17 08:50:13, Sebastian Noack wrote: > How about using classList? We don't need to care about compatibility with old IE > versions here. Done.
Also I don't think that we should push/publish this change until have the new image, or however communicate that it's not "comming soon" anymore but in beta. https://codereview.adblockplus.org/29341378/diff/29341478/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29341478/static/js/index.js#... static/js/index.js:38: if(content.className.indexOf("edge") == -1 || !window.navigator.userAgent) Nit: Missing space after "if". https://codereview.adblockplus.org/29341378/diff/29341478/static/js/index.js#... static/js/index.js:38: if(content.className.indexOf("edge") == -1 || !window.navigator.userAgent) If you put the check here below the UA check, it will be safe to use classList here as well.
New image uploaded https://codereview.adblockplus.org/29341378/diff/29341478/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29341478/static/js/index.js#... static/js/index.js:38: if(content.className.indexOf("edge") == -1 || !window.navigator.userAgent) On 2016/05/17 17:02:11, Sebastian Noack wrote: > If you put the check here below the UA check, it will be safe to use classList > here as well. Done. https://codereview.adblockplus.org/29341378/diff/29341478/static/js/index.js#... static/js/index.js:38: if(content.className.indexOf("edge") == -1 || !window.navigator.userAgent) On 2016/05/17 17:02:11, Sebastian Noack wrote: > Nit: Missing space after "if". Done.
You compressed the image with pngout, didn't you? https://codereview.adblockplus.org/29341378/diff/29342777/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29342777/static/js/index.js#... static/js/index.js:37: var content = document.getElementById("content"); We always assign variables where they are (first) used.
On 2016/05/18 12:06:39, Sebastian Noack wrote: > You compressed the image with pngout, didn't you? Yes I did > https://codereview.adblockplus.org/29341378/diff/29342777/static/js/index.js > File static/js/index.js (right): > > https://codereview.adblockplus.org/29341378/diff/29342777/static/js/index.js#... > static/js/index.js:37: var content = document.getElementById("content"); > We always assign variables where they are (first) used.
https://codereview.adblockplus.org/29341378/diff/29342777/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29342777/static/js/index.js#... static/js/index.js:37: var content = document.getElementById("content"); On 2016/05/18 12:06:38, Sebastian Noack wrote: > We always assign variables where they are (first) used. Done.
LGTM
Message was sent while issue was closed.
https://codereview.adblockplus.org/29341378/diff/29342784/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29342784/static/js/index.js#... static/js/index.js:42: if (match && parseFloat(match[1]) >= 14.14342 && 14 is a version of an EdgeHTML. 14342 is a Windows build number. Logically I think we should check only the Windows build number. EdgeHTML version might not be connected to the extensions functionality at all. See: https://blogs.windows.com/msedgedev/2015/09/21/understanding-versions-in-an-e...
Message was sent while issue was closed.
https://codereview.adblockplus.org/29341378/diff/29342784/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29341378/diff/29342784/static/js/index.js#... static/js/index.js:42: if (match && parseFloat(match[1]) >= 14.14342 && On 2016/05/18 12:53:15, Oleksandr wrote: > 14 is a version of an EdgeHTML. 14342 is a Windows build number. Logically I > think we should check only the Windows build number. EdgeHTML version might not > be connected to the extensions functionality at all. > > See: > https://blogs.windows.com/msedgedev/2015/09/21/understanding-versions-in-an-e... Yeah, I agree. However, it seems, the change already landed. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
