Issue 29458622: Issue 4920 - Add "Ready for Windows" section to adblockplus.org (Closed)
|
Created:
June 7, 2017, 2:53 p.m. by ire Modified:
June 26, 2017, 2:01 p.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/web.adblockplus.org Visibility:
Public. |
DescriptionIssue 4920 - Add "Ready for Windows" section to adblockplus.org
Patch Set 1 #
Total comments: 15
Patch Set 2 : Create notice class, use conditional rule #
Total comments: 14
Patch Set 3 : Style fixes #
MessagesTotal messages: 13
Thanks Ire :) https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:169: I know that I told you that you could show/hide based on a class applied to the body. Alternatively (it just occurred to me - sorry), you could test the page name in a conditional template tag. {% if page == "internet-explorer" %} bla bla bla {% endif %} https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:170: <div id="ready-for-windows-note"> Why not place the `id` on the `<p>` and drop the `<div>`? https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:170: <div id="ready-for-windows-note"> NIT: I don't think "note" is descriptive (Ack: You probably got the idea from the paragraph above). Perhaps "alert" (aka https://v4-alpha.getbootstrap.com/components/alerts/ http://foundation.zurb.com/sites/docs/v/5.5.3/components/alert_boxes.html ) or "notice". https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:171: <p>{{"Adblock Plus for Internet Explorer is supported by eyeo GmbH on the following editions of Windows 10 – Windows 10 Pro, Windows 10 Education and Windows 10 Enterprise. Adblock Plus for Internet Explorer is supported on the in-market supported servicing branches of Windows 10 including - Current Branch, Current Branch for Business and the following Long-Term Servicing branch: Windows 10."|translate("ready-for-windows-note")}}</p> Don't forget to `<fix>` product / brand names. https://codereview.adblockplus.org/29458622/diff/29458623/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29458622/diff/29458623/static/css/index.cs... static/css/index.css:711: #ready-for-windows-note { It looks like you are trying to match the style of the Adblock Browser alert (which also applies style by id). Why not refactor both to use a class instead? https://codereview.adblockplus.org/29458622/diff/29458623/static/css/index.cs... static/css/index.css:724: #content.internet-explorer #ready-for-windows-note { NIT: I don't think #content is necessary here.
https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:169: On 2017/06/07 17:48:11, juliandoucette wrote: > I know that I told you that you could show/hide based on a class applied to the > body. Alternatively (it just occurred to me - sorry), you could test the page > name in a conditional template tag. > > {% if page == "internet-explorer" %} > bla bla bla > {% endif %} Okay. I like that better than hiding with CSS so I'll refactor https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:170: <div id="ready-for-windows-note"> On 2017/06/07 17:48:11, juliandoucette wrote: > Why not place the `id` on the `<p>` and drop the `<div>`? Will do. https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:170: <div id="ready-for-windows-note"> On 2017/06/07 17:48:11, juliandoucette wrote: > NIT: I don't think "note" is descriptive (Ack: You probably got the idea from > the paragraph above). Perhaps "alert" (aka > https://v4-alpha.getbootstrap.com/components/alerts/ > http://foundation.zurb.com/sites/docs/v/5.5.3/components/alert_boxes.html ) or > "notice". Okay, I'll go with "ready-for-windows-notice". https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:171: <p>{{"Adblock Plus for Internet Explorer is supported by eyeo GmbH on the following editions of Windows 10 – Windows 10 Pro, Windows 10 Education and Windows 10 Enterprise. Adblock Plus for Internet Explorer is supported on the in-market supported servicing branches of Windows 10 including - Current Branch, Current Branch for Business and the following Long-Term Servicing branch: Windows 10."|translate("ready-for-windows-note")}}</p> On 2017/06/07 17:48:11, juliandoucette wrote: > Don't forget to `<fix>` product / brand names. Okay. I mentioned this in the translation string doc, but is this a rule we have? And which product names apply? Does "Windows 10 Pro" need to be fixed, or just "Windows"? How about "Internet Explorer"? https://codereview.adblockplus.org/29458622/diff/29458623/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29458622/diff/29458623/static/css/index.cs... static/css/index.css:711: #ready-for-windows-note { On 2017/06/07 17:48:11, juliandoucette wrote: > It looks like you are trying to match the style of the Adblock Browser alert > (which also applies style by id). Why not refactor both to use a class instead? Okay I will. I originally did it this way to keep with the style of the rest of the stylesheet, but I agree that using a class is better. https://codereview.adblockplus.org/29458622/diff/29458623/static/css/index.cs... static/css/index.css:724: #content.internet-explorer #ready-for-windows-note { On 2017/06/07 17:48:11, juliandoucette wrote: > NIT: I don't think #content is necessary here. Acknowledged.
@Tamara Can you please confirm or deny my answer below? https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:171: <p>{{"Adblock Plus for Internet Explorer is supported by eyeo GmbH on the following editions of Windows 10 – Windows 10 Pro, Windows 10 Education and Windows 10 Enterprise. Adblock Plus for Internet Explorer is supported on the in-market supported servicing branches of Windows 10 including - Current Branch, Current Branch for Business and the following Long-Term Servicing branch: Windows 10."|translate("ready-for-windows-note")}}</p> On 2017/06/08 13:41:26, ire wrote: > Okay. I mentioned this in the translation string doc, but is this a rule we > have? And which product names apply? Does "Windows 10 Pro" need to be fixed, or > just "Windows"? How about "Internet Explorer"? Good questions. I know we fix all of "Internet Explorer" and I think we fix all of "Windows 10 Pro". Can you confirm this @Tamara?
On 2017/06/08 21:12:30, juliandoucette wrote: > @Tamara > > Can you please confirm or deny my answer below? > > https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl > File includes/index.tmpl (right): > > https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... > includes/index.tmpl:171: <p>{{"Adblock Plus for Internet Explorer is supported > by eyeo GmbH on the following editions of Windows 10 – Windows 10 Pro, Windows > 10 Education and Windows 10 Enterprise. Adblock Plus for Internet Explorer is > supported on the in-market supported servicing branches of Windows 10 including > - Current Branch, Current Branch for Business and the following Long-Term > Servicing branch: Windows 10."|translate("ready-for-windows-note")}}</p> > On 2017/06/08 13:41:26, ire wrote: > > Okay. I mentioned this in the translation string doc, but is this a rule we > > have? And which product names apply? Does "Windows 10 Pro" need to be fixed, > or > > just "Windows"? How about "Internet Explorer"? > > Good questions. > > I know we fix all of "Internet Explorer" and I think we fix all of "Windows 10 > Pro". > > Can you confirm this @Tamara? Unfortunately I can't as I don't recall having "Windows" mentioned in any of the strings I've worked with. Besides, you guys are the ones coming up with the fix tags and deciding where to add them, not me.
On 2017/06/09 08:10:20, tamara wrote: > Unfortunately I can't as I don't recall having "Windows" mentioned in any of the > strings I've worked with. Besides, you guys are the ones coming up with the fix > tags and deciding where to add them, not me. Okay @tamara @juliandoucette I propose we <fix> the following in this string: - "Adblock Plus" - "eyeo GmbH" - "Windows 10" - "Internet Explorer" I had a look at the Microsoft site and it seems like this is the way they do it. See this page in English (https://www.microsoft.com/en-us/store/b/windows?icid=TopNavSoftwareWindows) and Arabic (https://www.microsoftstore.com/store/msmea/home/locale.ar_EG/ThemeID.31924400...). They have "Windows 10" fixed, but the version names, e.g. "Pro" or "Enterprise" are translated.
Thanks Tamara! Looking forward to your next patchset Ire :) https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:171: <p>{{"Adblock Plus for Internet Explorer is supported by eyeo GmbH on the following editions of Windows 10 – Windows 10 Pro, Windows 10 Education and Windows 10 Enterprise. Adblock Plus for Internet Explorer is supported on the in-market supported servicing branches of Windows 10 including - Current Branch, Current Branch for Business and the following Long-Term Servicing branch: Windows 10."|translate("ready-for-windows-note")}}</p> On 2017/06/08 21:12:30, juliandoucette wrote: > On 2017/06/08 13:41:26, ire wrote: > > Okay. I mentioned this in the translation string doc, but is this a rule we > > have? And which product names apply? Does "Windows 10 Pro" need to be fixed, > or > > just "Windows"? How about "Internet Explorer"? > > Good questions. > > I know we fix all of "Internet Explorer" and I think we fix all of "Windows 10 > Pro". > > Can you confirm this @Tamara? (Moving the discussion that was in reply back to review) Considering the lack of fix tags in the existing content, our current process, and the discussion going on in https://codereview.adblockplus.org/29422615/#msg14 I think we can safely ignore fix tags in this change.
(Removed Tamara from CC to avoid spamming her with code review) https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29458622/diff/29458623/includes/index.tmpl... includes/index.tmpl:169: On 2017/06/08 13:41:26, ire wrote: > On 2017/06/07 17:48:11, juliandoucette wrote: > > I know that I told you that you could show/hide based on a class applied to > the > > body. Alternatively (it just occurred to me - sorry), you could test the page > > name in a conditional template tag. > > > > {% if page == "internet-explorer" %} > > bla bla bla > > {% endif %} > > Okay. I like that better than hiding with CSS so I'll refactor :+1: Note: I asked Ollie if this applies to Edge too JIC. https://issues.adblockplus.org/ticket/4920#comment:19
> Considering the lack of fix tags in the existing content, our current process, > and the discussion going on in > https://codereview.adblockplus.org/29422615/#msg14 I think we can safely ignore > fix tags in this change. Noted. > Note: I asked Ollie if this applies to Edge too JIC. Seen his response.
Thanks Ire :) This one is close, just a few style issues and confusions. Regarding code-style, you can check out (and review) my stylelintrc [here](https://codereview.adblockplus.org/29360001/) https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:710: .notice { "Opening braces always go on their own line." -- https://adblockplus.org/coding-style#general https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:711: background-color: #ffffff; "CSS rule declaration order should follow the WordPress CSS Coding Standards." -- https://adblockplus.org/coding-style#html-css https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:714: box-sizing: border-box; What difference does this make? https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:715: color: #000000; "Use 3 character hexadecimal notation where possible." -- https://google.github.io/styleguide/htmlcssguide.html#Hexadecimal_Notation https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:719: margin: 10px 10px 30px; Why should only this notice be less wide and have more space beneath? https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:720: line-height: 1.4; Why should only this notice have comfortable line height? https://codereview.adblockplus.org/29458622/diff/29468555/static/css/main.css File static/css/main.css (left): https://codereview.adblockplus.org/29458622/diff/29468555/static/css/main.css... static/css/main.css:301: font-size: 16px; This is unnecessary.
> Regarding code-style, you can check out (and review) my stylelintrc > [here](https://codereview.adblockplus.org/29360001/) Will do. https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.css File static/css/index.css (right): https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:710: .notice { On 2017/06/20 16:20:16, juliandoucette wrote: > "Opening braces always go on their own line." > > -- https://adblockplus.org/coding-style#general Acknowledged. https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:711: background-color: #ffffff; On 2017/06/20 16:20:17, juliandoucette wrote: > "CSS rule declaration order should follow the WordPress CSS Coding Standards." > > -- https://adblockplus.org/coding-style#html-css Acknowledged. https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:714: box-sizing: border-box; On 2017/06/20 16:20:17, juliandoucette wrote: > What difference does this make? I had previously had `width: 100%` on this element and, since box-sizing wasn't defined anywhere in this CSS, the padding was making the box extend. But since I removed the width style, I don't need this either so will remove. https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:715: color: #000000; On 2017/06/20 16:20:16, juliandoucette wrote: > "Use 3 character hexadecimal notation where possible." > > -- https://google.github.io/styleguide/htmlcssguide.html#Hexadecimal_Notation Acknowledged. https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:719: margin: 10px 10px 30px; On 2017/06/20 16:20:17, juliandoucette wrote: > Why should only this notice be less wide and have more space beneath? It isn't meant to be. I had incorrectly copied the margin on #adblock-browser-notification from index-mobile.css. Will fix. I added more space beneath here because, for the other .notice (#adblock-browser-notification), there is already more space beneath due to the content that follows it. By adding 30px margin bottom in this case, I'm simulating the same amount of space the other notice has. https://codereview.adblockplus.org/29458622/diff/29468555/static/css/index.cs... static/css/index.css:720: line-height: 1.4; On 2017/06/20 16:20:17, juliandoucette wrote: > Why should only this notice have comfortable line height? I will move this style to the .notice class https://codereview.adblockplus.org/29458622/diff/29468555/static/css/main.css File static/css/main.css (left): https://codereview.adblockplus.org/29458622/diff/29468555/static/css/main.css... static/css/main.css:301: font-size: 16px; On 2017/06/20 16:20:17, juliandoucette wrote: > This is unnecessary. Acknowledged.
LGTM --- The line-height adjustment suggests that the default may not be readable. I think we should investigate that in a separate issue.
On 2017/06/26 10:00:57, juliandoucette wrote: > The line-height adjustment suggests that the default may not be readable. I > think we should investigate that in a separate issue. Yup you're right. I will create an issue for that 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
