Issue 29519636: Issue 5538 - Implement "Help" tab for new options page (Closed)
DescriptionThis review contains HTML/Semantics, strings, functional implementation and Help tab specific Styles.
Some general styles are still missing which will be the part of -> https://issues.adblockplus.org/ticket/5543
Patch Set 1 #
Total comments: 41
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
MessagesTotal messages: 8
I didn't specify a specific reviewer yet, will do as soon I'll know how occupied are you guys, meanwhile feel free to jump in if you will done with other reviews and will have time for this one next week. https://codereview.adblockplus.org/29519636/diff/29519641/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519636/diff/29519641/new-options.html#ne... new-options.html:336: <li><a id="weibo">Weibo</a></li> There still an open question regarding the Chinese social networks -> https://bitbucket.org/adblockplus/spec/issues/60/missin-weibo-icon-and-renren
This is ready for the review. https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:320: <li><a id="weibo">Weibo</a></li> Weibo icon is yet missing -> https://bitbucket.org/adblockplus/spec/issues/60/missin-weibo-icon-and-renren
Thanks Manvel! Here are my initial thoughts. I didn't address things like... - The link colours and underline styling - The layout of the sections (header on the left, content on the right) - Heading font sizes and styles ... Because I assumed these were styles that would be handled by the issue you mentioned. Apologies if I did address things that aren't specific to this issue. https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:168: "options_whitelist_description": { Unrelated, but I noticed that this id "options_whitelist_description" is a duplicate https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:363: "description": "'Section' title in Help tab", Should this description be "'Social' title in Help tab"? https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:371: "description": "Text in Help tab", "Text in Help tab" may be too vague here https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:298: <div id="content-help" role="tabpanel" aria-labelledby="tab-help"> There is no aria-hidden attribute attribute on the tabpanel to hide/show it when selected https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:319: <ul id="social-chinese"> NIT: I think the class name "social-zh" may be more accurate https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:320: <li><a id="weibo">Weibo</a></li> On 2017/08/23 17:36:10, saroyanm wrote: > Weibo icon is yet missing -> > https://bitbucket.org/adblockplus/spec/issues/60/missin-weibo-icon-and-renren Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:322: <p> This should be wrapped in a <strong> element, the text appears to be bold. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:906: #social ul Should there be the default padding-left on this element? https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:911: #social ul li The spacing between each list item is off/inexistent https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:916: #social ul li a The text isn't centered below the icon. Adding `text-align: center` here or on the `li` will fix that. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:916: #social ul li a I'm not sure that the styling of these links will be covered by the general style guide issue you mentioned (5543), because they appear to deviate. Should the styling of these then be handled in this issue? https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:925: width: 50px; Should this size be an em size instead? Feels like the size of the icons should be related to the font size in this case. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:934: mask: url(social/twitter.svg); Would using a plain background-image (or better yet an image in the markup) be better for browser compatibility here? Although you would have to specify the colour in the image itself and not CSS, it doesn't look like these images are being used elsewhere so that shouldn't be an issue. You know better than me on the browser requirements here so use your own judgment. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:934: mask: url(social/twitter.svg); NIT: quotes (") around the url
Thanks for the review Ire I think I have enough information for the next patch. https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:168: "options_whitelist_description": { On 2017/08/24 09:17:47, ire wrote: > Unrelated, but I noticed that this id "options_whitelist_description" is a > duplicate Well spotted, I'll fix that in the review with notification, as it's more related to whitelist tab. https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:363: "description": "'Section' title in Help tab", On 2017/08/24 09:17:47, ire wrote: > Should this description be "'Social' title in Help tab"? Not necessarily, I think the information should be enough for translator and it's consistent with other similar string descriptions. https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:371: "description": "Text in Help tab", On 2017/08/24 09:17:46, ire wrote: > "Text in Help tab" may be too vague here Yes it is I couldn't come up with good description, maybe "Email label in Help tab" https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:298: <div id="content-help" role="tabpanel" aria-labelledby="tab-help"> On 2017/08/24 09:17:47, ire wrote: > There is no aria-hidden attribute attribute on the tabpanel to hide/show it when > selected I agree, I think we should improve that separately, as this is out of scope of current review. Thanks for bringing this up, I'll tackle that either in https://issues.adblockplus.org/ticket/5543 or seprately as soon we will be ready with first viable product. https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:319: <ul id="social-chinese"> On 2017/08/24 09:17:47, ire wrote: > NIT: I think the class name "social-zh" may be more accurate I think we shouldn't put implementation details into the ID. Even we only show this for locale ZH that can change. https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:322: <p> On 2017/08/24 09:17:47, ire wrote: > This should be wrapped in a <strong> element, the text appears to be bold. agree https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:906: #social ul On 2017/08/24 09:17:47, ire wrote: > Should there be the default padding-left on this element? I think no, but this will be fixed with the sections separately, let's leave it right now as it is. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:911: #social ul li On 2017/08/24 09:17:47, ire wrote: > The spacing between each list item is off/inexistent Same here, I'm planing to touch this styles when styling the sections. This styles may or may not share common styles with other elements, not yet sure. So let's tackle that in the sections review. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:916: #social ul li a On 2017/08/24 09:17:47, ire wrote: > The text isn't centered below the icon. Adding `text-align: center` here or on > the `li` will fix that. I'll update. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:916: #social ul li a On 2017/08/24 09:17:48, ire wrote: > I'm not sure that the styling of these links will be covered by the general > style guide issue you mentioned (5543), because they appear to deviate. Should > the styling of these then be handled in this issue? I don't have strong opinion, as this styles are missing in the style guide. I'll stay away of addressing questionable style implementation in current review, as this review should address Strings and HTML, styles will be revisited separately, similar to other tabs. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:925: width: 50px; On 2017/08/24 09:17:47, ire wrote: > Should this size be an em size instead? Feels like the size of the icons should > be related to the font size in this case. Yes, it should I'll update this. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:934: mask: url(social/twitter.svg); On 2017/08/24 09:17:47, ire wrote: > Would using a plain background-image (or better yet an image in the markup) be > better for browser compatibility here? Although you would have to specify the > colour in the image itself and not CSS, it doesn't look like these images are > being used elsewhere so that shouldn't be an issue. Yes, I did it this way as the images are missing color and considering the fact that the browser support is not an issue in this case, as we target Modern Chrome and Firefox. Specifing SVG in HTML will affect readability, having specified as a background image, will require to produce more images for each color, for example if it will be required to change the color on hover, we will need to import another image if. So this solution seems to be good enough in my opinion for the launch as it provides flexibility and do not affect readability. > You know better than me on the browser requirements here so use your own > judgment. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:934: mask: url(social/twitter.svg); On 2017/08/24 09:17:47, ire wrote: > NIT: quotes (") around the url Well spotted I'll change.
New patch is ready for the review. https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:371: "description": "Text in Help tab", On 2017/08/24 13:22:27, saroyanm wrote: > On 2017/08/24 09:17:46, ire wrote: > > "Text in Help tab" may be too vague here > > Yes it is I couldn't come up with good description, maybe "Email label in Help > tab" Done. https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:322: <p> On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > This should be wrapped in a <strong> element, the text appears to be bold. > > agree Done. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:925: width: 50px; On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > Should this size be an em size instead? Feels like the size of the icons > should > > be related to the font size in this case. > > Yes, it should I'll update this. done. Root item font-size = 20px; https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:934: mask: url(social/twitter.svg); On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > NIT: quotes (") around the url > > Well spotted I'll change. Done.
Thanks Manvel. As most styling will be handled in a separate issue, then this LGTM (+ see a couple minor NITs) https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:168: "options_whitelist_description": { On 2017/08/24 13:22:27, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > Unrelated, but I noticed that this id "options_whitelist_description" is a > > duplicate > > Well spotted, I'll fix that in the review with notification, as it's more > related to whitelist tab. Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/locale/en-US/new-op... locale/en-US/new-options.json:363: "description": "'Section' title in Help tab", On 2017/08/24 13:22:27, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > Should this description be "'Social' title in Help tab"? > > Not necessarily, I think the information should be enough for translator and > it's consistent with other similar string descriptions. Acknowledged. My comment was because the "Section" was wrapped in quotes, implying that it was the title of the section. e.g. for the report section you wrote "'Support' section list item in Help tab". https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:298: <div id="content-help" role="tabpanel" aria-labelledby="tab-help"> On 2017/08/24 13:22:27, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > There is no aria-hidden attribute attribute on the tabpanel to hide/show it > when > > selected > > I agree, I think we should improve that separately, as this is out of scope of > current review. > Thanks for bringing this up, I'll tackle that either in > https://issues.adblockplus.org/ticket/5543 or seprately as soon we will be ready > with first viable product. Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/new-options.html#ne... new-options.html:319: <ul id="social-chinese"> On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > NIT: I think the class name "social-zh" may be more accurate > > I think we shouldn't put implementation details into the ID. Even we only show > this for locale ZH that can change. Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:906: #social ul On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > Should there be the default padding-left on this element? > > I think no, but this will be fixed with the sections separately, let's leave it > right now as it is. Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:911: #social ul li On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > The spacing between each list item is off/inexistent > > Same here, I'm planing to touch this styles when styling the sections. > This styles may or may not share common styles with other elements, not yet > sure. So let's tackle that in the sections review. Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:916: #social ul li a On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:48, ire wrote: > > I'm not sure that the styling of these links will be covered by the general > > style guide issue you mentioned (5543), because they appear to deviate. Should > > the styling of these then be handled in this issue? > > I don't have strong opinion, as this styles are missing in the style guide. I'll > stay away of addressing questionable style implementation in current review, as > this review should address Strings and HTML, styles will be revisited > separately, similar to other tabs. Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:925: width: 50px; On 2017/08/24 18:19:42, saroyanm wrote: > On 2017/08/24 13:22:28, saroyanm wrote: > > On 2017/08/24 09:17:47, ire wrote: > > > Should this size be an em size instead? Feels like the size of the icons > > should > > > be related to the font size in this case. > > > > Yes, it should I'll update this. > > done. > Root item font-size = 20px; Acknowledged. https://codereview.adblockplus.org/29519636/diff/29525593/skin/new-options.cs... skin/new-options.css:934: mask: url(social/twitter.svg); On 2017/08/24 13:22:28, saroyanm wrote: > On 2017/08/24 09:17:47, ire wrote: > > Would using a plain background-image (or better yet an image in the markup) be > > better for browser compatibility here? Although you would have to specify the > > colour in the image itself and not CSS, it doesn't look like these images are > > being used elsewhere so that shouldn't be an issue. > Yes, I did it this way as the images are missing color and considering the fact > that the browser support is not an issue in this case, as we target Modern > Chrome and Firefox. > > Specifing SVG in HTML will affect readability, having specified as a background > image, will require to produce more images for each color, for example if it > will be required to change the color on hover, we will need to import another > image if. So this solution seems to be good enough in my opinion for the launch > as it provides flexibility and do not affect readability. > > > You know better than me on the browser requirements here so use your own > > judgment. > Acknowledged. https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.cs... skin/new-options.css:901: html[lang="zh"] #social-general NIT: We don't need the `html` part of this selector https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.cs... skin/new-options.css:919: text-align: center Missing semi-colon
Thanks Ire. New patch uploaded https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.cs... skin/new-options.css:901: html[lang="zh"] #social-general On 2017/08/25 08:52:50, ire wrote: > NIT: We don't need the `html` part of this selector I don't have strong opinion, but I think it makes more readable considering the :not selector before. https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.cs... skin/new-options.css:919: text-align: center On 2017/08/25 08:52:50, ire wrote: > Missing semi-colon Done.
We're done then :) https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29519636/diff/29526741/skin/new-options.cs... skin/new-options.css:901: html[lang="zh"] #social-general On 2017/08/25 10:54:37, saroyanm wrote: > On 2017/08/25 08:52:50, ire wrote: > > NIT: We don't need the `html` part of this selector > > I don't have strong opinion, but I think it makes more readable considering the > :not selector before. Okay, leave it as it is then. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
