Issue 29502647: Issue 5482 - Sidebar and about ABP dialog (Closed)
|
Created:
Aug. 1, 2017, 4:09 p.m. by saroyanm Modified:
Aug. 28, 2017, 12:08 p.m. Reviewers:
juliandoucette CC:
Thomas Greiner, ire Visibility:
Public. |
Patch Set 1 : #
Total comments: 56
Patch Set 2 : #
Total comments: 79
Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : Rebased #
Total comments: 1
Patch Set 5 : Fixed tabulation #
Total comments: 29
Patch Set 6 : #Patch Set 7 : Rebased #Patch Set 8 : Styleguide changes #
Total comments: 2
Patch Set 9 : Fixed nit #
MessagesTotal messages: 24
This is ready for the review, references to styleguide and implementation specific links to the specification document can be find in the Issue ticket. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:334: <div id="dialog-content-about" class="dialog-content"> Styles for Dialog is missing, I'll create separate review to address all Dialog styles separately. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:338: <button class="i18n_options_close primary" data-action="close-dialog"></button> I'll update all other buttons to primary or secondary accordingly in separate review. I only updated current buttons to test visually. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.js File new-options.js (left): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.js#oldc... new-options.js:553: function openDocLink(id) This is not being used anymore. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.js#oldc... new-options.js:1308: function updateShareLink() We don't have share dialog in "new" new version of the options page. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:60: font-size: 48px; I'll change font-sizes as soon they are updated in the style guide - as was discussed in the meeting, the plan is to use EMs for the font-sizes as well. https://bitbucket.org/adblockplus/spec/issues/55/use-ems-for-the-font-sizes-i...
Here are my first impressions. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:38: <p class="i18n_options_page_title_2"></p> NIT: ~"Settings" is not a paragraph. I suggest using a <br> and <span> as demonstrated [here](https://www.w3.org/TR/html5/common-idioms.html#sub-head) https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:67: <a id="contribute" class="i18n_options_footer_contribute button secondary" I think we should add an external link icon to this anchor because buttons usually link internally and this links externally. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:71: <button id="about" class="i18n_options_footer_about" data-action="open-dialog" data-dialog="about"></button> I think that this link is very misleading. It looks like a link to our about page (missing an external link icon). https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:334: <div id="dialog-content-about" class="dialog-content"> On 2017/08/10 12:04:48, saroyanm wrote: > Styles for Dialog is missing, I'll create separate review to address all Dialog > styles separately. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:338: <button class="i18n_options_close primary" data-action="close-dialog"></button> On 2017/08/10 12:04:48, saroyanm wrote: > I'll update all other buttons to primary or secondary accordingly in separate > review. I only updated current buttons to test visually. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:60: font-size: 48px; On 2017/08/10 12:04:48, saroyanm wrote: > I'll change font-sizes as soon they are updated in the style guide - as was > discussed in the meeting, the plan is to use EMs for the font-sizes as well. > > https://bitbucket.org/adblockplus/spec/issues/55/use-ems-for-the-font-sizes-i... Acknowledged. Thank you for pointing this out. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:92: button.primary, NIT: I prefer adding .button to <button>s and styling *just* .button here and .button.primary below. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:138: a Did you mean to style all <a> here? (It doesn't look like it) https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:197: html[dir="rtl"] #sidebar header NIT: You could have just margin: 0 2em; above https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:211: margin: 0; NIT: I thought we'd agreed against unitless values? https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:219: #sidebar-logo NIT: It's kindof inconsistent that you are styling this image so specifically and those nav and footer so generally. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:252: cursor: pointer; Is there a reason that you are using <spans> inside of <li> here instead of <a>? https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:259: text-decoration: none; NIT/Note: You could default to none and add decoration to `#content a` or otherwise. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:266: margin: auto 0.8em; NIT: I don't think auto does anything here. If I am correct than it may be misleading. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:272: text-shadow: 0px 0px 1px #888; NIT/Note: This doesn't look so hot on [lowdpi, chromium, debain] https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:283: footer Did you mean to style all <footer>s here? (It doesn't look like it. The footer element is quite general and these styles are quite specific.) https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:310: max-width: 46.3em; I think that you (or the specifier) may have made a miscalculation (e.g. rounding and/or not counting borders). It looks like you (or the specifier) are trying to reach a total (including the sidebar and spacing) max-width of 1280px; and you are currently sitting at 1286px.
https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:38: <p class="i18n_options_page_title_2"></p> On 2017/08/16 23:42:31, juliandoucette wrote: > NIT: ~"Settings" is not a paragraph. I suggest using a <br> and <span> as > demonstrated [here](https://www.w3.org/TR/html5/common-idioms.html#sub-head) I agree. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:92: button.primary, On 2017/08/16 23:42:32, juliandoucette wrote: > NIT: I prefer adding .button to <button>s and styling *just* .button here and > .button.primary below. No, I don't think that we should have class button on the element <button> if that what you mean. I used .button to style non button elements as .button link in this case "Contribute" which semantically is anchor, but looks like a "button". https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:138: a On 2017/08/16 23:42:32, juliandoucette wrote: > Did you mean to style all <a> here? > > (It doesn't look like it) Yes, I did, though I think some of the styles are misleading, don't remember why I end up having them here as well. I'll update it to look more generic. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:197: html[dir="rtl"] #sidebar header On 2017/08/16 23:42:32, juliandoucette wrote: > NIT: You could have just margin: 0 2em; above I did it to save space, mostly for the translations. The fonts are to huge here also considering that probably this is expected to reach the edge (0.3em from edge, judging from the style-guide). In general I think we should decrease the font-size, but I want to keep it for later and don't stress style-guide updates, but rather revisit similar issues later on with Jeen (including the active tab highlight question you have brought up below). https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:211: margin: 0; On 2017/08/16 23:42:32, juliandoucette wrote: > NIT: I thought we'd agreed against unitless values? Noted, will fix. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:219: #sidebar-logo On 2017/08/16 23:42:32, juliandoucette wrote: > NIT: It's kindof inconsistent that you are styling this image so specifically > and those nav and footer so generally. I agree, I'll change. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:252: cursor: pointer; On 2017/08/16 23:42:31, juliandoucette wrote: > Is there a reason that you are using <spans> inside of <li> here instead of <a>? No this is styles updated on top of old implementation of tabs, I agree that having <a> instead of <span>s is correct here. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:259: text-decoration: none; On 2017/08/16 23:42:32, juliandoucette wrote: > NIT/Note: You could default to none and add decoration to `#content a` or > otherwise. I don't think we need text-decoration specified for any of a element according to the styleguides/specs I'll update implementation defaulting that for <a> tag in general https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:266: margin: auto 0.8em; On 2017/08/16 23:42:32, juliandoucette wrote: > NIT: I don't think auto does anything here. If I am correct than it may be > misleading. It's not, well spotted. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:272: text-shadow: 0px 0px 1px #888; On 2017/08/16 23:42:32, juliandoucette wrote: > NIT/Note: This doesn't look so hot on [lowdpi, chromium, debain] I agree, I think we should enhance this with Jeen, update styleguide and the implementation, as it's not specified in the style guide, anyway I think this shouldn't be blocker for this review. But thanks for heads-up. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:283: footer On 2017/08/16 23:42:32, juliandoucette wrote: > Did you mean to style all <footer>s here? > > (It doesn't look like it. The footer element is quite general and these styles > are quite specific.) Currently we only have 1 footer, so I did it general. but I'll make this more specific. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:310: max-width: 46.3em; On 2017/08/16 23:42:32, juliandoucette wrote: > I think that you (or the specifier) may have made a miscalculation (e.g. > rounding and/or not counting borders). It looks like you (or the specifier) are > trying to reach a total (including the sidebar and spacing) max-width of 1280px; > and you are currently sitting at 1286px. Yes, you are right this should be "border-box"
https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:67: <a id="contribute" class="i18n_options_footer_contribute button secondary" On 2017/08/16 23:42:31, juliandoucette wrote: > I think we should add an external link icon to this anchor because buttons > usually link internally and this links externally. Right, but as mentioned in several places in current review, I don't think that this should be blocker for current review. Same as other points in CSS file we will revisit this small enhancements which are not blockers. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:71: <button id="about" class="i18n_options_footer_about" data-action="open-dialog" data-dialog="about"></button> On 2017/08/16 23:42:31, juliandoucette wrote: > I think that this link is very misleading. It looks like a link to our about > page (missing an external link icon). Same as above.
Thanks Manvel, I think that I've responded to anything that you may have wanted my feedback on before proceeding. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:67: <a id="contribute" class="i18n_options_footer_contribute button secondary" On 2017/08/18 10:49:29, saroyanm wrote: > On 2017/08/16 23:42:31, juliandoucette wrote: > > I think we should add an external link icon to this anchor because buttons > > usually link internally and this links externally. > > Right, but as mentioned in several places in current review, I don't think that > this should be blocker for current review. Same as other points in CSS file we > will revisit this small enhancements which are not blockers. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:71: <button id="about" class="i18n_options_footer_about" data-action="open-dialog" data-dialog="about"></button> On 2017/08/18 10:49:29, saroyanm wrote: > On 2017/08/16 23:42:31, juliandoucette wrote: > > I think that this link is very misleading. It looks like a link to our about > > page (missing an external link icon). > > Same as above. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:92: button.primary, On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > NIT: I prefer adding .button to <button>s and styling *just* .button here and > > .button.primary below. > > No, I don't think that we should have class button on the element <button> if > that what you mean. > > I used .button to style non button elements as .button link in this case > "Contribute" which semantically is anchor, but looks like a "button". Acknowledged. A <link> can look like a <button> just like a <button> can look like a <link> (e.g. the button and link in the sidebar). And you can assign classes accordingly or not {link: .button, button: .link}. I should point out that you did not apply them accordingly in the sidebar because the <button> doesn't have a .link class. But this is only a consistency NIT. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:197: html[dir="rtl"] #sidebar header On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > NIT: You could have just margin: 0 2em; above > > I did it to save space, mostly for the translations. The fonts are to huge here > also considering that probably this is expected to reach the edge (0.3em from > edge, judging from the style-guide). > > In general I think we should decrease the font-size, but I want to keep it for > later and don't stress style-guide updates, but rather revisit similar issues > later on with Jeen (including the active tab highlight question you have brought > up below). Acknowledged. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:272: text-shadow: 0px 0px 1px #888; On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > NIT/Note: This doesn't look so hot on [lowdpi, chromium, debain] > > I agree, I think we should enhance this with Jeen, update styleguide and the > implementation, as it's not specified in the style guide, anyway I think this > shouldn't be blocker for this review. > But thanks for heads-up. Acknowledged.
Thanks Julian for reviewing, the new patch is ready for review as well, please still apply it to the same patch of General tab review. https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:38: <p class="i18n_options_page_title_2"></p> On 2017/08/16 23:42:31, juliandoucette wrote: > NIT: ~"Settings" is not a paragraph. I suggest using a <br> and <span> as > demonstrated [here](https://www.w3.org/TR/html5/common-idioms.html#sub-head) I tried that solution, but the markup and implementation started to look funny IMO. The problem is that the current way the translation strings are implemented is that we do specify the translation strings inside of "classes" of the element. So in order to achieve the result I should have implemented smth like this: <h1> <span class="i18n_options_page_title_1"> <br> <span class="i18n_options_page_title_2"> </h1> After that override the h1 style and apply styles inside of h1. I still can do that if you will think it still make sense, but I currently just used <h2> instead of paragraph. Let me know if that's fine with you or you want me to go the other way anyway. I think this way the markup is easier to read as well. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:60: font-size: 48px; On 2017/08/16 23:42:31, juliandoucette wrote: > On 2017/08/10 12:04:48, saroyanm wrote: > > I'll change font-sizes as soon they are updated in the style guide - as was > > discussed in the meeting, the plan is to use EMs for the font-sizes as well. > > > > > https://bitbucket.org/adblockplus/spec/issues/55/use-ems-for-the-font-sizes-i... > > Acknowledged. > > Thank you for pointing this out. Done, but still I'm not really convinced with the specified sizes, this is something we should run through and adjust with Jeen as soon we ship this. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:92: button.primary, On 2017/08/18 11:17:04, juliandoucette wrote: > On 2017/08/18 10:23:13, saroyanm wrote: > > On 2017/08/16 23:42:32, juliandoucette wrote: > > > NIT: I prefer adding .button to <button>s and styling *just* .button here > and > > > .button.primary below. > > > > No, I don't think that we should have class button on the element <button> if > > that what you mean. > > > > I used .button to style non button elements as .button link in this case > > "Contribute" which semantically is anchor, but looks like a "button". > > Acknowledged. > > A <link> can look like a <button> just like a <button> can look like a <link> > (e.g. the button and link in the sidebar). And you can assign classes > accordingly or not {link: .button, button: .link}. I should point out that you > did not apply them accordingly in the sidebar because the <button> doesn't have > a .link class. But this is only a consistency NIT. I agree, I thought you proposing to have button.button style, but now I see what you mean, I think I made this more consitent now by introducing, button.link https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:138: a On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > Did you mean to style all <a> here? > > > > (It doesn't look like it) > > Yes, I did, though I think some of the styles are misleading, don't remember why > I end up having them here as well. > > I'll update it to look more generic. Done. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:211: margin: 0; On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > NIT: I thought we'd agreed against unitless values? > > Noted, will fix. Done. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:219: #sidebar-logo On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > NIT: It's kindof inconsistent that you are styling this image so specifically > > and those nav and footer so generally. > > I agree, I'll change. On other hand, nav and footer are not unique elements, but sidebar-logo is, so being specific in this case I think make sense. I keet the implementation if you agree. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:252: cursor: pointer; On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:31, juliandoucette wrote: > > Is there a reason that you are using <spans> inside of <li> here instead of > <a>? > > No this is styles updated on top of old implementation of tabs, I agree that > having <a> instead of <span>s is correct here. Done. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:259: text-decoration: none; On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > NIT/Note: You could default to none and add decoration to `#content a` or > > otherwise. > > I don't think we need text-decoration specified for any of a element according > to the styleguides/specs I'll update implementation defaulting that for <a> tag > in general Done. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:266: margin: auto 0.8em; On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > NIT: I don't think auto does anything here. If I am correct than it may be > > misleading. > > It's not, well spotted. Done. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:283: footer On 2017/08/18 10:23:13, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > Did you mean to style all <footer>s here? > > > > (It doesn't look like it. The footer element is quite general and these styles > > are quite specific.) > > Currently we only have 1 footer, so I did it general. > but I'll make this more specific. Done. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:310: max-width: 46.3em; On 2017/08/18 10:23:12, saroyanm wrote: > On 2017/08/16 23:42:32, juliandoucette wrote: > > I think that you (or the specifier) may have made a miscalculation (e.g. > > rounding and/or not counting borders). It looks like you (or the specifier) > are > > trying to reach a total (including the sidebar and spacing) max-width of > 1280px; > > and you are currently sitting at 1286px. > > Yes, you are right this should be "border-box" Done. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (left): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ol... new-options.html:84: <div id="content-wrapper"> The only change below is that I removed this container, as it was misleading and all the changes below are just indentation changes, no other change is done. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:78: <main> I also changed this element, as we were already touching it's styles in this review, considering that it's indirectly also connected with this review and that helped me to make more consistent selector as well.
(Feedback before I review the new Patchset.) https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29510576/new-options.html#ne... new-options.html:38: <p class="i18n_options_page_title_2"></p> On 2017/08/18 12:44:27, saroyanm wrote: > On 2017/08/16 23:42:31, juliandoucette wrote: > > NIT: ~"Settings" is not a paragraph. I suggest using a <br> and <span> as > > demonstrated [here](https://www.w3.org/TR/html5/common-idioms.html#sub-head) > > I tried that solution, but the markup and implementation started to look funny > IMO. > > The problem is that the current way the translation strings are implemented is > that we do specify the translation strings inside of "classes" of the element. > So in order to achieve the result I should have implemented smth like this: > > <h1> > <span class="i18n_options_page_title_1"> <br> > <span class="i18n_options_page_title_2"> > </h1> > > After that override the h1 style and apply styles inside of h1. > > I still can do that if you will think it still make sense, but I currently just > used <h2> instead of paragraph. Let me know if that's fine with you or you want > me to go the other way anyway. > I think this way the markup is easier to read as well. Acknowledged. I think <p> is better than <h2> here. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:60: font-size: 48px; On 2017/08/18 12:44:28, saroyanm wrote: > On 2017/08/16 23:42:31, juliandoucette wrote: > > On 2017/08/10 12:04:48, saroyanm wrote: > > > I'll change font-sizes as soon they are updated in the style guide - as was > > > discussed in the meeting, the plan is to use EMs for the font-sizes as well. > > > > > > > > > https://bitbucket.org/adblockplus/spec/issues/55/use-ems-for-the-font-sizes-i... > > > > Acknowledged. > > > > Thank you for pointing this out. > > Done, but still I'm not really convinced with the specified sizes, this is > something we should run through and adjust with Jeen as soon we ship this. Acknowledged. I agree. These sizes are huge. Perhaps we should ask the data team if we can have something to inform Jeen about our average user's screen size. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:92: button.primary, On 2017/08/18 12:44:28, saroyanm wrote: > On 2017/08/18 11:17:04, juliandoucette wrote: > > On 2017/08/18 10:23:13, saroyanm wrote: > > > On 2017/08/16 23:42:32, juliandoucette wrote: > > > > NIT: I prefer adding .button to <button>s and styling *just* .button here > > and > > > > .button.primary below. > > > > > > No, I don't think that we should have class button on the element <button> > if > > > that what you mean. > > > > > > I used .button to style non button elements as .button link in this case > > > "Contribute" which semantically is anchor, but looks like a "button". > > > > Acknowledged. > > > > A <link> can look like a <button> just like a <button> can look like a <link> > > (e.g. the button and link in the sidebar). And you can assign classes > > accordingly or not {link: .button, button: .link}. I should point out that you > > did not apply them accordingly in the sidebar because the <button> doesn't > have > > a .link class. But this is only a consistency NIT. > > I agree, I thought you proposing to have button.button style, but now I see what > you mean, I think I made this more consitent now by introducing, button.link Acknowledged. FWIW: I was originally proposing to have button.button. And I added another different suggestion after you rejected my button.button NIT suggestion. https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... skin/new-options.css:219: #sidebar-logo On 2017/08/18 12:44:28, saroyanm wrote: > On 2017/08/18 10:23:13, saroyanm wrote: > > On 2017/08/16 23:42:32, juliandoucette wrote: > > > NIT: It's kindof inconsistent that you are styling this image so > specifically > > > and those nav and footer so generally. > > > > I agree, I'll change. > > On other hand, nav and footer are not unique elements, but sidebar-logo is, so > being specific in this case I think make sense. > I keet the implementation if you agree. Acknowledged. I agree. Good thinking. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (left): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ol... new-options.html:84: <div id="content-wrapper"> On 2017/08/18 12:44:29, saroyanm wrote: > The only change below is that I removed this container, as it was misleading and > all the changes below are just indentation changes, no other change is done. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:78: <main> On 2017/08/18 12:44:29, saroyanm wrote: > I also changed this element, as we were already touching it's styles in this > review, considering that it's indirectly also connected with this review and > that helped me to make more consistent selector as well. Acknowledged.
Finished reviewing latest Patchset. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", NIT: These are headings not titles. IDK if it makes a difference to translators. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> This logo is pixelated at 1x. I suggest aligning it better and/or scaling it up to resolve this issue. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:38: <h2 class="i18n_options_page_title_2"></h2> I think <p> is better than <h2> here. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:48: <a class="i18n_options_tab_general"></a> NIT: I don't think that text-shadow is enough to clearly show selection (e.g. select a tab with your mouse and then tab with your keyboard to the next item.) I suggest adding some sort of background or border colour change too. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:53: <a class="i18n_options_tab_whitelist"></a> Tabbing seems to function differently between Chrome/Firefox here. - Chrome: Pressing tab selects the next sidebar item - Firefox: Pressing tab selects an item in the main content area I think this can be fixed via tabindex? https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:67: <a id="contribute" NIT: I don't think that the border thickness change on hover (which doesn't, and should, happen when you tab to this button) is enough. I suggest changing the background colour. And differentiating between hover and active states. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:321: <div id="dialog" role="dialog" aria-hidden="true"> I think you can/should take one of two approaches. # Approach 1 (roughly) <aside role="dialog"> <header> <h1>Modal heading</h1> <button class="close" aria-label="Close"> <span aria-hidden="true">×</span> </button> </header> <p>Modal content</p> <footer> <button class="primary">Close</button> </footer> </aside> # Approach 2 (roughly) <div class="modal" role="dailog"> <div class="modal-header"> <h5 class="modal-title">Modal title</h5> <button type="button" class="close" aria-label="Close"> <span aria-hidden="true">×</span> </button> </div> <div class="modal-body"> <p>Modal body text goes here.</p> </div> <div class="modal-footer"> <button type="button" class="primary">Close</button> </div> </div> </div> https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:323: <span id="dialog-title"> There are no headings in this header :/ https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:798: function updateTabLinks() NIT: Why not add these HREFs directly to new-options.html? https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:800: let tabs = document.querySelectorAll("[role='tab']"); NIT: Isn't this asking for trouble? e.g. if we add non-sidebar tabs in the future Suggest: "#sidebar .tabs a" (or equivalent) https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:880: getDocLink("contribute", (link) => NIT: It seems like these should be batched somewhere e.g. setDocLinks([ [DOC_ID, DOM_ID, ATTR_NAME], [DOC_ID, DOM_ID, ATTR_NAME] ]); Perhaps automatically via HTML attributes or className style. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:55: font-weight: 300; This thin body font requires aliasing and is hard to read at this size; especially at lowDPI. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:94: font-size: 1.25em; It seem strange to make anchors slightly larger than body text :/ https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:100: border:none; NIT: Missing space, + I thought we agreed on border: 0? https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:106: button.primary, These seem like styles that should apply to button, .button without .primary and .secondary? https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:124: cursor: pointer; I think this is redundant?
Thanks for reviwing Julian I answered your comments, I'm looking for your reply to them so I can start to prepare new patch. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On 2017/08/21 14:10:33, juliandoucette wrote: > NIT: These are headings not titles. IDK if it makes a difference to translators. I don't think that title is misleading here, but I agree that this shouldn't be translated. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> On 2017/08/21 14:10:33, juliandoucette wrote: > This logo is pixelated at 1x. I suggest aligning it better and/or scaling it up > to resolve this issue. I can't test that, looks fine for me, please note it's a SVG image, it shouldn't be pixalated in theory. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:38: <h2 class="i18n_options_page_title_2"></h2> On 2017/08/21 14:10:33, juliandoucette wrote: > I think <p> is better than <h2> here. I'll change it back. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:48: <a class="i18n_options_tab_general"></a> On 2017/08/21 14:10:33, juliandoucette wrote: > NIT: I don't think that text-shadow is enough to clearly show selection (e.g. > select a tab with your mouse and then tab with your keyboard to the next item.) > I suggest adding some sort of background or border colour change too. I agree, As mentioned here -> https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... the plan is that we will enhance this with Jeen after, as it's currently not specified, this shouldn't be a blocker for this review IMO. After launching this changes I'll be able to make this available for the web and Jeen can test and propose enhancements in align with us. I don't think it will be right choice to involve her in current review. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:53: <a class="i18n_options_tab_whitelist"></a> On 2017/08/21 14:10:34, juliandoucette wrote: > Tabbing seems to function differently between Chrome/Firefox here. > > - Chrome: Pressing tab selects the next sidebar item > - Firefox: Pressing tab selects an item in the main content area > > I think this can be fixed via tabindex? Can't reproduce that :/ https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:67: <a id="contribute" On 2017/08/21 14:10:33, juliandoucette wrote: > NIT: I don't think that the border thickness change on hover (which doesn't, and > should, happen when you tab to this button) is enough. I suggest changing the > background colour. And differentiating between hover and active states. I think I did it consistently with the style guide, let me know if not. Styleguide enhacements should be addressed in the bitbucket. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:321: <div id="dialog" role="dialog" aria-hidden="true"> On 2017/08/21 14:10:33, juliandoucette wrote: > I think you can/should take one of two approaches. > > # Approach 1 (roughly) > > <aside role="dialog"> > <header> > <h1>Modal heading</h1> > <button class="close" aria-label="Close"> > <span aria-hidden="true">×</span> > </button> > </header> > <p>Modal content</p> > <footer> > <button class="primary">Close</button> > </footer> > </aside> > > # Approach 2 (roughly) > > <div class="modal" role="dailog"> > <div class="modal-header"> > <h5 class="modal-title">Modal title</h5> > <button type="button" class="close" aria-label="Close"> > <span aria-hidden="true">×</span> > </button> > </div> > <div class="modal-body"> > <p>Modal body text goes here.</p> > </div> > <div class="modal-footer"> > <button type="button" class="primary">Close</button> > </div> > </div> > </div> Thanks for bringing this: I think using aside is arguable, but I do agree that we need to use h tags for the title. We can discuss improvements of the dialogs where we gonna touch them all here -> https://issues.adblockplus.org/ticket/5548 This review is only adding dialog consistent with implementation of other dialogs. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:323: <span id="dialog-title"> On 2017/08/21 14:10:34, juliandoucette wrote: > There are no headings in this header :/ No there are no, it's shouldn't be part of current review though. Could be part of this -> https://issues.adblockplus.org/ticket/5548 https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:798: function updateTabLinks() On 2017/08/21 14:10:34, juliandoucette wrote: > NIT: Why not add these HREFs directly to new-options.html? To avoid duplications, we do specify the target in datasets, so this should be specified in 1 place, please note we don't optimizing here for SEO. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:800: let tabs = document.querySelectorAll("[role='tab']"); On 2017/08/21 14:10:34, juliandoucette wrote: > NIT: Isn't this asking for trouble? e.g. if we add non-sidebar tabs in the > future > > Suggest: "#sidebar .tabs a" (or equivalent) Why is this trouble ? No, this logic I think should be general, but not sidebar specific, until you intentionally making it sidebar specific, I don't think that it's a case, here. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:880: getDocLink("contribute", (link) => On 2017/08/21 14:10:34, juliandoucette wrote: > NIT: It seems like these should be batched somewhere e.g. > > setDocLinks([ > [DOC_ID, DOM_ID, ATTR_NAME], > [DOC_ID, DOM_ID, ATTR_NAME] > ]); > > Perhaps automatically via HTML attributes or className style. Yes it's a plan for future. We might also have a ticket for that. If not we can create one. Again, future enhancement or at least separate from this review. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:55: font-weight: 300; On 2017/08/21 14:10:35, juliandoucette wrote: > This thin body font requires aliasing and is hard to read at this size; > especially at lowDPI. As mentioned, on other file, styleguide enhancements, suggestions should be part of bitbucket repository. This review reflects the styleguide on top of old implementation. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:94: font-size: 1.25em; On 2017/08/21 14:10:34, juliandoucette wrote: > It seem strange to make anchors slightly larger than body text :/ I agree, this is not what styleguide says. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:100: border:none; On 2017/08/21 14:10:34, juliandoucette wrote: > NIT: Missing space, + I thought we agreed on border: 0? Good point. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:106: button.primary, On 2017/08/21 14:10:34, juliandoucette wrote: > These seem like styles that should apply to button, .button without .primary and > .secondary? It, can but in that case we should reset some styles that my apply to .link and other buttons in general, in case of other buttons we should do that separately according to their reviews. I agree with you that we should make this styles more generic as they looks more like new options page specific default buttons style. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:124: cursor: pointer; On 2017/08/21 14:10:34, juliandoucette wrote: > I think this is redundant? Why ?
Thanks Manvel, here are my replies. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On 2017/08/21 15:20:21, saroyanm wrote: > On 2017/08/21 14:10:33, juliandoucette wrote: > > NIT: These are headings not titles. IDK if it makes a difference to > translators. > > I don't think that title is misleading here, but I agree that this shouldn't be > translated. I don't know what you mean by "this shouldn't be translated". https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> On 2017/08/21 15:20:21, saroyanm wrote: > On 2017/08/21 14:10:33, juliandoucette wrote: > > This logo is pixelated at 1x. I suggest aligning it better and/or scaling it > up > > to resolve this issue. > > I can't test that, looks fine for me, please note it's a SVG image, it shouldn't > be pixalated in theory. - Test via 72 or 96 DPI monitor in the office - SVG doesn't magically eliminate pixilation at small sizes - Aliasing matters - Sub-pixels matter TL;DR NIT I think we can do better. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:38: <h2 class="i18n_options_page_title_2"></h2> On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:33, juliandoucette wrote: > > I think <p> is better than <h2> here. > > I'll change it back. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:48: <a class="i18n_options_tab_general"></a> On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:33, juliandoucette wrote: > > NIT: I don't think that text-shadow is enough to clearly show selection (e.g. > > select a tab with your mouse and then tab with your keyboard to the next > item.) > > I suggest adding some sort of background or border colour change too. > > I agree, > As mentioned here -> > https://codereview.adblockplus.org/29502647/diff/29510576/skin/new-options.cs... > the plan is that we will enhance this with Jeen after, as it's currently not > specified, this shouldn't be a blocker for this review IMO. > > After launching this changes I'll be able to make this available for the web and > Jeen can test and propose enhancements in align with us. I don't think it will > be right choice to involve her in current review. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:53: <a class="i18n_options_tab_whitelist"></a> On 2017/08/21 15:20:21, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > Tabbing seems to function differently between Chrome/Firefox here. > > > > - Chrome: Pressing tab selects the next sidebar item > > - Firefox: Pressing tab selects an item in the main content area > > > > I think this can be fixed via tabindex? > > Can't reproduce that :/ What happens in your browsers? https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:67: <a id="contribute" On 2017/08/21 15:20:21, saroyanm wrote: > I think I did it consistently with the style guide, let me know if not. > Styleguide enhacements should be addressed in the bitbucket. I don't see an active state for the secondary button in the latest styleguide in the spec? https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:321: <div id="dialog" role="dialog" aria-hidden="true"> On 2017/08/21 15:20:21, saroyanm wrote: > On 2017/08/21 14:10:33, juliandoucette wrote: > > I think you can/should take one of two approaches. > > > > # Approach 1 (roughly) > > > > <aside role="dialog"> > > <header> > > <h1>Modal heading</h1> > > <button class="close" aria-label="Close"> > > <span aria-hidden="true">×</span> > > </button> > > </header> > > <p>Modal content</p> > > <footer> > > <button class="primary">Close</button> > > </footer> > > </aside> > > > > # Approach 2 (roughly) > > > > <div class="modal" role="dailog"> > > <div class="modal-header"> > > <h5 class="modal-title">Modal title</h5> > > <button type="button" class="close" aria-label="Close"> > > <span aria-hidden="true">×</span> > > </button> > > </div> > > <div class="modal-body"> > > <p>Modal body text goes here.</p> > > </div> > > <div class="modal-footer"> > > <button type="button" class="primary">Close</button> > > </div> > > </div> > > </div> > > Thanks for bringing this: I think using aside is arguable, but I do agree that > we need to use h tags for the title. > > We can discuss improvements of the dialogs where we gonna touch them all here -> > https://issues.adblockplus.org/ticket/5548 > > This review is only adding dialog consistent with implementation of other > dialogs. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:323: <span id="dialog-title"> On 2017/08/21 15:20:21, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > There are no headings in this header :/ > > No there are no, it's shouldn't be part of current review though. > Could be part of this -> https://issues.adblockplus.org/ticket/5548 Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:798: function updateTabLinks() On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > NIT: Why not add these HREFs directly to new-options.html? > > To avoid duplications, we do specify the target in datasets, so this should be > specified in 1 place, please note we don't optimizing here for SEO. NIT: You could *just* set a.href then and: tabList.querySelector("li[aria-controls='content-" + tabId + "']") OR tabList.querySelector("li#tab-" + tabId + "']") OR tabList.querySelector("a[href='#" + tabId + "']").parentElement elsewhere then. But I don't insist. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:800: let tabs = document.querySelectorAll("[role='tab']"); On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > NIT: Isn't this asking for trouble? e.g. if we add non-sidebar tabs in the > > future > > > > Suggest: "#sidebar .tabs a" (or equivalent) > > Why is this trouble ? No, this logic I think should be general, but not sidebar > specific, until you intentionally making it sidebar specific, I don't think that > it's a case, here. Acknowledged. Assuming that you want this to happen to all (not just sidebar) tabs in this document in the future. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:880: getDocLink("contribute", (link) => On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > NIT: It seems like these should be batched somewhere e.g. > > > > setDocLinks([ > > [DOC_ID, DOM_ID, ATTR_NAME], > > [DOC_ID, DOM_ID, ATTR_NAME] > > ]); > > > > Perhaps automatically via HTML attributes or className style. > > Yes it's a plan for future. We might also have a ticket for that. If not we can > create one. Again, future enhancement or at least separate from this review. Acknowledged. Will you check this? https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:55: font-weight: 300; On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:35, juliandoucette wrote: > > This thin body font requires aliasing and is hard to read at this size; > > especially at lowDPI. > > As mentioned, on other file, styleguide enhancements, suggestions should be part > of bitbucket repository. > > This review reflects the styleguide on top of old implementation. I can't find "300" or "thin" in the styleguide or spec anywhere? It just says "Regular" (which is normally 400)? https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:94: font-size: 1.25em; On 2017/08/21 15:20:23, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > It seem strange to make anchors slightly larger than body text :/ > > I agree, this is not what styleguide says. I can't tell if you are saying that you or the styleguide made a mistake. I can't find it specified in the spec or styleguide that link text should be larger. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:106: button.primary, On 2017/08/21 15:20:23, saroyanm wrote: > It, can but in that case we should reset some styles that my apply to .link and > other buttons in general, in case of other buttons we should do that separately > according to their reviews. Do button on other parts of this page or in other reviews function differently? Or do you just want to address general button styles in a separate review? It looks like general button styles are being addressed here. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:124: cursor: pointer; On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > I think this is redundant? > > Why ? I was mistaken; sorry. I thought that this style was unnecessary because a, button.link applied it. Anyway, shouldn't this (cursor:pointer) be part of button.primary AND button.secondary above then? (Adapted accordingly if you change the above style based on my comments.)
https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On 2017/08/21 16:07:23, juliandoucette wrote: > On 2017/08/21 15:20:21, saroyanm wrote: > > On 2017/08/21 14:10:33, juliandoucette wrote: > > > NIT: These are headings not titles. IDK if it makes a difference to > > translators. > > > > I don't think that title is misleading here, but I agree that this shouldn't > be > > translated. > > I don't know what you mean by "this shouldn't be translated". This translation string is redundant: "Adblock <strong>Plus</strong>" shouldn't be translated. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> On 2017/08/21 16:07:24, juliandoucette wrote: > On 2017/08/21 15:20:21, saroyanm wrote: > > On 2017/08/21 14:10:33, juliandoucette wrote: > > > This logo is pixelated at 1x. I suggest aligning it better and/or scaling it > > up > > > to resolve this issue. > > > > I can't test that, looks fine for me, please note it's a SVG image, it > shouldn't > > be pixalated in theory. > > - Test via 72 or 96 DPI monitor in the office > - SVG doesn't magically eliminate pixilation at small sizes > - Aliasing matters > - Sub-pixels matter > > TL;DR NIT I think we can do better. I don't know what needs/want to be done here ? Are you proposing changing the SVG image ? Anyway I'll be in the office tomorrow and we can have a look, this shouldn't be blocker for the next patch I think. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:53: <a class="i18n_options_tab_whitelist"></a> On 2017/08/21 16:07:23, juliandoucette wrote: > On 2017/08/21 15:20:21, saroyanm wrote: > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > Tabbing seems to function differently between Chrome/Firefox here. > > > > > > - Chrome: Pressing tab selects the next sidebar item > > > - Firefox: Pressing tab selects an item in the main content area > > > > > > I think this can be fixed via tabindex? > > > > Can't reproduce that :/ > > What happens in your browsers? Same you explained for "Chrome". https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:67: <a id="contribute" On 2017/08/21 16:07:24, juliandoucette wrote: > On 2017/08/21 15:20:21, saroyanm wrote: > > I think I did it consistently with the style guide, let me know if not. > > Styleguide enhacements should be addressed in the bitbucket. > > I don't see an active state for the secondary button in the latest styleguide in > the spec? Yes, active state for secondary button is missing from the specs. Anyway this shouldn't be blocker for this review, adding another state should be simple separately as soon Jeen will update the design to reflect the state. We will bring this during the tomorrow meeting. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:798: function updateTabLinks() On 2017/08/21 16:07:24, juliandoucette wrote: > On 2017/08/21 15:20:22, saroyanm wrote: > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > NIT: Why not add these HREFs directly to new-options.html? > > > > To avoid duplications, we do specify the target in datasets, so this should be > > specified in 1 place, please note we don't optimizing here for SEO. > > NIT: You could *just* set a.href then and: > > tabList.querySelector("li[aria-controls='content-" + tabId + "']") > > OR > > tabList.querySelector("li#tab-" + tabId + "']") > > OR > > tabList.querySelector("a[href='#" + tabId + "']").parentElement > > elsewhere then. But I don't insist. > I agree with you, I'll change this. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:880: getDocLink("contribute", (link) => On 2017/08/21 16:07:25, juliandoucette wrote: > On 2017/08/21 15:20:22, saroyanm wrote: > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > NIT: It seems like these should be batched somewhere e.g. > > > > > > setDocLinks([ > > > [DOC_ID, DOM_ID, ATTR_NAME], > > > [DOC_ID, DOM_ID, ATTR_NAME] > > > ]); > > > > > > Perhaps automatically via HTML attributes or className style. > > > > Yes it's a plan for future. We might also have a ticket for that. If not we > can > > create one. Again, future enhancement or at least separate from this review. > > Acknowledged. > > Will you check this? https://issues.adblockplus.org/ticket/4856 https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:55: font-weight: 300; On 2017/08/21 16:07:25, juliandoucette wrote: > On 2017/08/21 15:20:22, saroyanm wrote: > > On 2017/08/21 14:10:35, juliandoucette wrote: > > > This thin body font requires aliasing and is hard to read at this size; > > > especially at lowDPI. > > > > As mentioned, on other file, styleguide enhancements, suggestions should be > part > > of bitbucket repository. > > > > This review reflects the styleguide on top of old implementation. > > I can't find "300" or "thin" in the styleguide or spec anywhere? > > It just says "Regular" (which is normally 400)? Yes, it's using same fonts value assignment as Acceptable Ads. So yes it's missing in styleguide, but Jeen wanted to have same fonts and probably enhancements here should reflect enhancements also on our websites/products. But I agree we need to have for that case separate global styles to adjust to. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:94: font-size: 1.25em; On 2017/08/21 16:07:25, juliandoucette wrote: > On 2017/08/21 15:20:23, saroyanm wrote: > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > It seem strange to make anchors slightly larger than body text :/ > > > > I agree, this is not what styleguide says. > > I can't tell if you are saying that you or the styleguide made a mistake. I > can't find it specified in the spec or styleguide that link text should be > larger. I'm saying that I'll fix that, style guide specify 1.25em on Body, while I specify 20px, I probably need to specify 20px on HTML and use 1.25em on body. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:106: button.primary, On 2017/08/21 16:07:25, juliandoucette wrote: > On 2017/08/21 15:20:23, saroyanm wrote: > > It, can but in that case we should reset some styles that my apply to .link > and > > other buttons in general, in case of other buttons we should do that > separately > > according to their reviews. > > Do button on other parts of this page or in other reviews function differently? > Or do you just want to address general button styles in a separate review? > It looks like general button styles are being addressed here. Yes, they look, but if change here will break them we will fix in separate review as we anyway gonna revisit them, so long story short: I'll update this implementation in next patch. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:124: cursor: pointer; On 2017/08/21 16:07:25, juliandoucette wrote: > On 2017/08/21 15:20:22, saroyanm wrote: > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > I think this is redundant? > > > > Why ? > > I was mistaken; sorry. I thought that this style was unnecessary because a, > button.link applied it. > > Anyway, shouldn't this (cursor:pointer) be part of button.primary AND > button.secondary above then? (Adapted accordingly if you change the above style > based on my comments.) Yes, I think it should.
There are couple of things that are missing for final discussion, the discussions that are irrelevant for the patch I mentioned as so, for example the ABP icon issue, which I can't test right now. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On 2017/08/21 16:46:09, saroyanm wrote: > On 2017/08/21 16:07:23, juliandoucette wrote: > > On 2017/08/21 15:20:21, saroyanm wrote: > > > On 2017/08/21 14:10:33, juliandoucette wrote: > > > > NIT: These are headings not titles. IDK if it makes a difference to > > > translators. > > > > > > I don't think that title BTW I initially thought that you were referencing to the ID, but i agree that heading would made more sense here.
Following up. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On 2017/08/21 17:02:55, saroyanm wrote: > On 2017/08/21 16:46:09, saroyanm wrote: > > On 2017/08/21 16:07:23, juliandoucette wrote: > > > On 2017/08/21 15:20:21, saroyanm wrote: > > > > On 2017/08/21 14:10:33, juliandoucette wrote: > > > > > NIT: These are headings not titles. IDK if it makes a difference to > > > > translators. > > > > > > > > I don't think that title > BTW I initially thought that you were referencing to the ID, but i agree that > heading would made more sense here. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> On 2017/08/21 16:46:10, saroyanm wrote: > I don't know what needs/want to be done here ? > Are you proposing changing the SVG image ? Yes. I'm suggesting that we may be able to change the SVG and/or export it into another format to improve it's quality on lowDPI screens. > Anyway I'll be in the office tomorrow and we can have a look, this shouldn't be > blocker for the next patch I think. I agree. And I would recommend that Jeen (or someone equivalent) investigates this issue before launch. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:53: <a class="i18n_options_tab_whitelist"></a> On 2017/08/21 16:46:10, saroyanm wrote: > On 2017/08/21 16:07:23, juliandoucette wrote: > > On 2017/08/21 15:20:21, saroyanm wrote: > > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > > Tabbing seems to function differently between Chrome/Firefox here. > > > > > > > > - Chrome: Pressing tab selects the next sidebar item > > > > - Firefox: Pressing tab selects an item in the main content area > > > > > > > > I think this can be fixed via tabindex? > > > > > > Can't reproduce that :/ > > > > What happens in your browsers? > > Same you explained for "Chrome". It's definitely an issue on FF for OS X. And adding tabindex to the <a> fixes it. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:67: <a id="contribute" On 2017/08/21 16:46:09, saroyanm wrote: > On 2017/08/21 16:07:24, juliandoucette wrote: > > On 2017/08/21 15:20:21, saroyanm wrote: > > > I think I did it consistently with the style guide, let me know if not. > > > Styleguide enhacements should be addressed in the bitbucket. > > > > I don't see an active state for the secondary button in the latest styleguide > in > > the spec? > > Yes, active state for secondary button is missing from the specs. > > Anyway this shouldn't be blocker for this review, adding another state should be > simple separately as soon Jeen will update the design to reflect the state. > > We will bring this during the tomorrow meeting. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:880: getDocLink("contribute", (link) => On 2017/08/21 16:46:13, saroyanm wrote: > On 2017/08/21 16:07:25, juliandoucette wrote: > > On 2017/08/21 15:20:22, saroyanm wrote: > > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > > NIT: It seems like these should be batched somewhere e.g. > > > > > > > > setDocLinks([ > > > > [DOC_ID, DOM_ID, ATTR_NAME], > > > > [DOC_ID, DOM_ID, ATTR_NAME] > > > > ]); > > > > > > > > Perhaps automatically via HTML attributes or className style. > > > > > > Yes it's a plan for future. We might also have a ticket for that. If not we > > can > > > create one. Again, future enhancement or at least separate from this review. > > > > Acknowledged. > > > > Will you check this? > > https://issues.adblockplus.org/ticket/4856 Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:55: font-weight: 300; On 2017/08/21 16:46:13, saroyanm wrote: > On 2017/08/21 16:07:25, juliandoucette wrote: > > On 2017/08/21 15:20:22, saroyanm wrote: > > > On 2017/08/21 14:10:35, juliandoucette wrote: > > > > This thin body font requires aliasing and is hard to read at this size; > > > > especially at lowDPI. > > > > > > As mentioned, on other file, styleguide enhancements, suggestions should be > > part > > > of bitbucket repository. > > > > > > This review reflects the styleguide on top of old implementation. > > > > I can't find "300" or "thin" in the styleguide or spec anywhere? > > > > It just says "Regular" (which is normally 400)? > > Yes, it's using same fonts value assignment as Acceptable Ads. > So yes it's missing in styleguide, but Jeen wanted to have same fonts and > probably enhancements here should reflect enhancements also on our > websites/products. > > But I agree we need to have for that case separate global styles to adjust to. Acknowledged. I think it's worth bringing up. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:94: font-size: 1.25em; On 2017/08/21 16:46:13, saroyanm wrote: > On 2017/08/21 16:07:25, juliandoucette wrote: > > On 2017/08/21 15:20:23, saroyanm wrote: > > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > > It seem strange to make anchors slightly larger than body text :/ > > > > > > I agree, this is not what styleguide says. > > > > I can't tell if you are saying that you or the styleguide made a mistake. I > > can't find it specified in the spec or styleguide that link text should be > > larger. > > I'm saying that I'll fix that, style guide specify 1.25em on Body, while I > specify 20px, I probably need to specify 20px on HTML and use 1.25em on body. Acknowledged. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:106: button.primary, On 2017/08/21 16:46:13, saroyanm wrote: > On 2017/08/21 16:07:25, juliandoucette wrote: > > On 2017/08/21 15:20:23, saroyanm wrote: > > > It, can but in that case we should reset some styles that my apply to .link > > and > > > other buttons in general, in case of other buttons we should do that > > separately > > > according to their reviews. > > > > Do button on other parts of this page or in other reviews function > differently? > > Or do you just want to address general button styles in a separate review? > > It looks like general button styles are being addressed here. > > Yes, they look, but if change here will break them we will fix in separate > review as we anyway gonna revisit them, so long story short: I'll update this > implementation in next patch. Acknowledged.
This is ready for review. https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29502647/diff/29519601/locale/en-US/new-op... locale/en-US/new-options.json:7: "description": "Page title in navigation sidebar", On 2017/08/22 10:10:43, juliandoucette wrote: > On 2017/08/21 17:02:55, saroyanm wrote: > > On 2017/08/21 16:46:09, saroyanm wrote: > > > On 2017/08/21 16:07:23, juliandoucette wrote: > > > > On 2017/08/21 15:20:21, saroyanm wrote: > > > > > On 2017/08/21 14:10:33, juliandoucette wrote: > > > > > > NIT: These are headings not titles. IDK if it makes a difference to > > > > > translators. > > > > > > > > > > I don't think that title > > BTW I initially thought that you were referencing to the ID, but i agree that > > heading would made more sense here. > > Acknowledged. Done. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:38: <h2 class="i18n_options_page_title_2"></h2> On 2017/08/21 16:07:23, juliandoucette wrote: > On 2017/08/21 15:20:22, saroyanm wrote: > > On 2017/08/21 14:10:33, juliandoucette wrote: > > > I think <p> is better than <h2> here. > > > > I'll change it back. > > Acknowledged. Done. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:53: <a class="i18n_options_tab_whitelist"></a> On 2017/08/22 10:10:44, juliandoucette wrote: > On 2017/08/21 16:46:10, saroyanm wrote: > > On 2017/08/21 16:07:23, juliandoucette wrote: > > > On 2017/08/21 15:20:21, saroyanm wrote: > > > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > > > Tabbing seems to function differently between Chrome/Firefox here. > > > > > > > > > > - Chrome: Pressing tab selects the next sidebar item > > > > > - Firefox: Pressing tab selects an item in the main content area > > > > > > > > > > I think this can be fixed via tabindex? > > > > > > > > Can't reproduce that :/ > > > > > > What happens in your browsers? > > > > Same you explained for "Chrome". > > It's definitely an issue on FF for OS X. And adding tabindex to the <a> fixes > it. This implementation is changed please let me know if you still have different behavior in different browsers. Beforehand we had a mixture, of tabindex set on <li> + <a> focusable element, so the behavior might have been messed up. https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.js#newc... new-options.js:798: function updateTabLinks() On 2017/08/21 16:46:12, saroyanm wrote: > On 2017/08/21 16:07:24, juliandoucette wrote: > > On 2017/08/21 15:20:22, saroyanm wrote: > > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > > NIT: Why not add these HREFs directly to new-options.html? > > > > > > To avoid duplications, we do specify the target in datasets, so this should > be > > > specified in 1 place, please note we don't optimizing here for SEO. > > > > NIT: You could *just* set a.href then and: > > > > tabList.querySelector("li[aria-controls='content-" + tabId + "']") > > > > OR > > > > tabList.querySelector("li#tab-" + tabId + "']") > > > > OR > > > > tabList.querySelector("a[href='#" + tabId + "']").parentElement > > > > elsewhere then. But I don't insist. > > > > I agree with you, I'll change this. I've updated the tabs implementation. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:94: font-size: 1.25em; On 2017/08/22 10:10:44, juliandoucette wrote: > On 2017/08/21 16:46:13, saroyanm wrote: > > On 2017/08/21 16:07:25, juliandoucette wrote: > > > On 2017/08/21 15:20:23, saroyanm wrote: > > > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > > > It seem strange to make anchors slightly larger than body text :/ > > > > > > > > I agree, this is not what styleguide says. > > > > > > I can't tell if you are saying that you or the styleguide made a mistake. I > > > can't find it specified in the spec or styleguide that link text should be > > > larger. > > > > I'm saying that I'll fix that, style guide specify 1.25em on Body, while I > > specify 20px, I probably need to specify 20px on HTML and use 1.25em on body. > > Acknowledged. Neverming I was wrong, I shouldn't style <a> here, this is sidebar button/button.link specific style. Also additional adjustments of the section will be done here -> https://issues.adblockplus.org/ticket/5543 https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:100: border:none; On 2017/08/21 15:20:22, saroyanm wrote: > On 2017/08/21 14:10:34, juliandoucette wrote: > > NIT: Missing space, + I thought we agreed on border: 0? > > Good point. Done. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:106: button.primary, On 2017/08/22 10:10:44, juliandoucette wrote: > On 2017/08/21 16:46:13, saroyanm wrote: > > On 2017/08/21 16:07:25, juliandoucette wrote: > > > On 2017/08/21 15:20:23, saroyanm wrote: > > > > It, can but in that case we should reset some styles that my apply to > .link > > > and > > > > other buttons in general, in case of other buttons we should do that > > > separately > > > > according to their reviews. > > > > > > Do button on other parts of this page or in other reviews function > > differently? > > > Or do you just want to address general button styles in a separate review? > > > It looks like general button styles are being addressed here. > > > > Yes, they look, but if change here will break them we will fix in separate > > review as we anyway gonna revisit them, so long story short: I'll update this > > implementation in next patch. > > Acknowledged. Done. https://codereview.adblockplus.org/29502647/diff/29519601/skin/new-options.cs... skin/new-options.css:124: cursor: pointer; On 2017/08/21 16:46:13, saroyanm wrote: > On 2017/08/21 16:07:25, juliandoucette wrote: > > On 2017/08/21 15:20:22, saroyanm wrote: > > > On 2017/08/21 14:10:34, juliandoucette wrote: > > > > I think this is redundant? > > > > > > Why ? > > > > I was mistaken; sorry. I thought that this style was unnecessary because a, > > button.link applied it. > > > > Anyway, shouldn't this (cursor:pointer) be part of button.primary AND > > button.secondary above then? (Adapted accordingly if you change the above > style > > based on my comments.) > > Yes, I think it should. Done. https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.cs... skin/new-options.css:60: font-size: 1rem; I started to use REMs because to avoide compounding problems and browser support is not an issue here.
https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29519601/new-options.html#ne... new-options.html:53: <a class="i18n_options_tab_whitelist"></a> On 2017/08/23 13:35:44, saroyanm wrote: > This implementation is changed please let me know if you still have different > behavior in different browsers. Beforehand we had a mixture, of tabindex set on > <li> + <a> focusable element, so the behavior might have been messed up. See my comment [here](https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#newcode48). https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.cs... skin/new-options.css:60: font-size: 1rem; On 2017/08/23 13:35:46, saroyanm wrote: > I started to use REMs because to avoide compounding problems and browser support > is not an issue here. Yes, but you set the font-size on the html element which overrides the browsers font-size property and makes this change inaccessible. I suggest that you unset the font-size on the HTML element and re-calculate REM values accordingly if you choose to use REMs. https://codereview.adblockplus.org/29502647/diff/29524759/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524759/skin/new-options.cs... skin/new-options.css:277: #sidebar footer This overlaps the above nav at heights below ~700. I suggest making the fixed navbar scrollable or hiding it below screen-height 768px. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> NIT/Suggest: Missing alt text e.g. "Adblock Plus logo", "Stop sign shaped Adblock Plus logo" or ~aria-hidden="true" https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:43: <nav> For reference below: ARIA tabs spec: https://www.w3.org/TR/wai-aria-practices/#tabpanel ARIA example #1: https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html ARIA example #2: https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-2/tabs.html https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:45: role="tablist" data-action="switch-tab" NIT: Missing aria-label https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:47: <li> NIT: These <li> should have the "presentation" role (See https://www.w3.org/TR/wai-aria-practices/#h-presentation_role). https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:48: <a href="#general" class="i18n_options_tab_general" role="tab" aria-selected="true" aria-controls="content-general"></a> NIT: Apparently I was wrong about the tab key function ( Sorry :( ). The arrow keys should navigate between tabs and the tab key should navigate away (like it does on OS X Firefox). Therefore, I think you should put tabindex="-1" as seen in [ARIA example #1](https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html) https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:48: <a href="#general" class="i18n_options_tab_general" role="tab" aria-selected="true" aria-controls="content-general"></a> Note: Regarding the use of anchors vs buttons for this purpose: # A case for <a> I think <a> is more appropriate if we stack the tabpanels vertically so that they could be navigated to by a.href if JavaScript is disabled. # A case for <button> I think <button> is more appropriate if we do not automatically activate tabs when pressing arrow keys because a <button> can be activated by Space *or* Enter and an <a> can *only* be activated by Enter. --- Therefore I think the choice should depend on first whether or not we support no-js (we don't) and second whether or not we automatically activate tabs via selection (we do). My conclusion is that <a> seems suitable here. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:61: <footer> Note: The same logic as explained above regarding <a> vs <button> applies below. https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:59: font-weight: 300; Note: This issue is still non-blocking and unresolved (I suggested 400). https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:295: max-width: 46.3rem; Note: I didn't re-calculate all of the horizontal widths to see if they add up now. https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:295: max-width: 46.3rem; Note: The spec doesn't seem to say anything about this max-width.
Thanks for the notes Julian, I think I have enough information to prepare new patch. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> On 2017/08/23 18:11:10, juliandoucette wrote: > NIT/Suggest: Missing alt text e.g. "Adblock Plus logo", "Stop sign shaped > Adblock Plus logo" or ~aria-hidden="true" There is no easy way to translate that. I'll hardcode for now. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:45: role="tablist" data-action="switch-tab" On 2017/08/23 18:11:09, juliandoucette wrote: > NIT: Missing aria-label I think providing role="tablist" information should be enough. That what screen reader see: "Tablist with 4 item 'General' tab selected". If you have suggestion on your mind to enhance this information let me know. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:47: <li> On 2017/08/23 18:11:09, juliandoucette wrote: > NIT: These <li> should have the "presentation" role (See > https://www.w3.org/TR/wai-aria-practices/#h-presentation_role). I'll add that, didn't know about this use case. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:48: <a href="#general" class="i18n_options_tab_general" role="tab" aria-selected="true" aria-controls="content-general"></a> On 2017/08/23 18:11:10, juliandoucette wrote: > NIT: Apparently I was wrong about the tab key function ( Sorry :( ). The arrow > keys should navigate between tabs and the tab key should navigate away (like it > does on OS X Firefox). Therefore, I think you should put tabindex="-1" as seen > in [ARIA example > #1](https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html) Agree. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:48: <a href="#general" class="i18n_options_tab_general" role="tab" aria-selected="true" aria-controls="content-general"></a> On 2017/08/23 18:11:10, juliandoucette wrote: > Note: > > Regarding the use of anchors vs buttons for this purpose: > > # A case for <a> > > I think <a> is more appropriate if we stack the tabpanels vertically so that > they could be navigated to by a.href if JavaScript is disabled. > > # A case for <button> > > I think <button> is more appropriate if we do not automatically activate tabs > when pressing arrow keys because a <button> can be activated by Space *or* Enter > and an <a> can *only* be activated by Enter. > > --- > > Therefore I think the choice should depend on first whether or not we support > no-js (we don't) and second whether or not we automatically activate tabs via > selection (we do). My conclusion is that <a> seems suitable here. I can't follow your thoughts here, but: In this case I think using <a> is more appropriate because it inform user that it also change the location "#advanced", so user know that can navigate to advanced section of the page. Also irregardless what element is used it should be changed by arrows. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:61: <footer> On 2017/08/23 18:11:11, juliandoucette wrote: > Note: The same logic as explained above regarding <a> vs <button> applies below. I don't think it applies, below in one case we actually have a link which lead to another page and Button which opens dialog. So it's not inconsistent it's just different element which I think fits their semantics. https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:59: font-weight: 300; On 2017/08/23 18:11:11, juliandoucette wrote: > Note: This issue is still non-blocking and unresolved (I suggested 400). I'll change to 400 https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:295: max-width: 46.3rem; On 2017/08/23 18:11:11, juliandoucette wrote: > Note: The spec doesn't seem to say anything about this max-width. It does -> https://bytebucket.org/adblockplus/spec/raw/f20ab3573d020065e415dca85961f3143... It's states that the width is 46.3em https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:295: max-width: 46.3rem; On 2017/08/23 18:11:11, juliandoucette wrote: > Note: I didn't re-calculate all of the horizontal widths to see if they add up > now. Not sure what you mean, but it's border-box element, so paddings are included in size calculation, if that what you were referring to.
Following up. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:45: role="tablist" data-action="switch-tab" On 2017/08/23 20:57:53, saroyanm wrote: > On 2017/08/23 18:11:09, juliandoucette wrote: > > NIT: Missing aria-label > > I think providing role="tablist" information should be enough. > > That what screen reader see: > "Tablist with 4 item 'General' tab selected". > > If you have suggestion on your mind to enhance this information let me know. Nothing comes to mind. I only mentioned it because the example that I linked above recommended it. I don't think it matters very much. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:48: <a href="#general" class="i18n_options_tab_general" role="tab" aria-selected="true" aria-controls="content-general"></a> On 2017/08/23 20:57:53, saroyanm wrote: > On 2017/08/23 18:11:10, juliandoucette wrote: > > Note: > > > > Regarding the use of anchors vs buttons for this purpose: > > > > # A case for <a> > > > > I think <a> is more appropriate if we stack the tabpanels vertically so that > > they could be navigated to by a.href if JavaScript is disabled. > > > > # A case for <button> > > > > I think <button> is more appropriate if we do not automatically activate tabs > > when pressing arrow keys because a <button> can be activated by Space *or* > Enter > > and an <a> can *only* be activated by Enter. > > > > --- > > > > Therefore I think the choice should depend on first whether or not we support > > no-js (we don't) and second whether or not we automatically activate tabs via > > selection (we do). My conclusion is that <a> seems suitable here. > > I can't follow your thoughts here, but: > In this case I think using <a> is more appropriate because it inform user that > it also change the location "#advanced", so user know that can navigate to > advanced section of the page. > > Also irregardless what element is used it should be changed by arrows. I agree. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:61: <footer> On 2017/08/23 20:57:53, saroyanm wrote: > On 2017/08/23 18:11:11, juliandoucette wrote: > > Note: The same logic as explained above regarding <a> vs <button> applies > below. > > I don't think it applies, below in one case we actually have a link which lead > to another page and Button which opens dialog. So it's not inconsistent it's > just different element which I think fits their semantics. I agree. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:63: <a id="contribute" Why does this link to https://adblockplus.org/redirect?link=contribute and not https://adblockplus.org/contribute? https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:295: max-width: 46.3rem; On 2017/08/23 20:57:54, saroyanm wrote: > On 2017/08/23 18:11:11, juliandoucette wrote: > > Note: I didn't re-calculate all of the horizontal widths to see if they add up > > now. > > Not sure what you mean, but it's border-box element, so paddings are included in > size calculation, if that what you were referring to. Yes. I was referring to my previous comment where I pointed out that the total horizontal width of the whole layout was probably wider than you/Jeen intended and speculated about why that was. I was only pointing out that I didn't re-check the width.
I think this is ready for the review. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:36: <img id="sidebar-logo" src="skin/abp-logo.svg"> On 2017/08/23 20:57:52, saroyanm wrote: > On 2017/08/23 18:11:10, juliandoucette wrote: > > NIT/Suggest: Missing alt text e.g. "Adblock Plus logo", "Stop sign shaped > > Adblock Plus logo" or ~aria-hidden="true" > > There is no easy way to translate that. > I'll hardcode for now. Done. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:47: <li> On 2017/08/23 20:57:53, saroyanm wrote: > On 2017/08/23 18:11:09, juliandoucette wrote: > > NIT: These <li> should have the "presentation" role (See > > https://www.w3.org/TR/wai-aria-practices/#h-presentation_role). > > I'll add that, didn't know about this use case. Done. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:48: <a href="#general" class="i18n_options_tab_general" role="tab" aria-selected="true" aria-controls="content-general"></a> On 2017/08/23 20:57:52, saroyanm wrote: > On 2017/08/23 18:11:10, juliandoucette wrote: > > NIT: Apparently I was wrong about the tab key function ( Sorry :( ). The arrow > > keys should navigate between tabs and the tab key should navigate away (like > it > > does on OS X Firefox). Therefore, I think you should put tabindex="-1" as seen > > in [ARIA example > > #1](https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html) > > Agree. Done. https://codereview.adblockplus.org/29502647/diff/29524769/new-options.html#ne... new-options.html:63: <a id="contribute" On 2017/08/23 22:00:46, juliandoucette wrote: > Why does this link to https://adblockplus.org/redirect?link=contribute and not > https://adblockplus.org/contribute All links are redirect links, so whenever a links needs to be changed we will not change in all our products, but redirect link. See -> https://bitbucket.org/adblockplus/spec/src/23f2fb52e8d176d6fc4104e69755991e65... https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524769/skin/new-options.cs... skin/new-options.css:59: font-weight: 300; On 2017/08/23 20:57:54, saroyanm wrote: > On 2017/08/23 18:11:11, juliandoucette wrote: > > Note: This issue is still non-blocking and unresolved (I suggested 400). > > I'll change to 400 I didn't touch the font size, as I still have a clarification question -> https://bitbucket.org/adblockplus/spec/issues/69 and question about which font to use -> https://bitbucket.org/adblockplus/spec/issues/69#comment-39300762 Let's keep yet with old plan, not make this blocker yet, we will fix this in current review or separately depending on the ticket resolution.
Rebased.
This is ready for the review, but I wish to finish this review soon if possible, I tried to work on other issues, but noticed that they are dependent on this one, please note that this will mean additional effort of rebasing on top of the patch, so please review this one with priority if possible. During the review please consider: * Please avoid introducing additional design changes which are not crucial for the initial launch (anyway Jeen is on vacation, so I think it's a reasonable request anyway). * In case of personal preferences please provide desired implementation, if it make sense I don't have problem implementing that if that's not contradict the style guide. * @Julian considering the fact that you have got sick unfortunately, can you please let me know in case you will not be able to review this change, so maybe we can ask @Ire or @Greiner to do that. Thanks in advance.
On 2017/08/27 16:13:11, saroyanm wrote: > * @Julian considering the fact that you have got sick unfortunately, can you > please let me know in case you will not be able to review this change, so maybe > we can ask @Ire or @Greiner to do that. > > Thanks in advance. I tried to review this tonight but Rietveld wouldn't let me (server errors). I won't be available to review again until tomorrow evening. If you want it done before then than please ask for help from ire and/or greiner.
LGTM + NITs Another Patchset or Review will be necessary to address the REM font-size issue. https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.cs... skin/new-options.css:60: font-size: 1rem; On 2017/08/23 18:11:08, juliandoucette wrote: > Yes, but you set the font-size on the html element which overrides the browsers > font-size property and makes this change inaccessible. I suggest that you unset > the font-size on the HTML element and re-calculate REM values accordingly if you > choose to use REMs. NIT: This is still an issue. https://codereview.adblockplus.org/29502647/diff/29528555/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29528555/new-options.html#ne... new-options.html:38: Adblock <strong>Plus</strong> NIT: The weight of strong is different in FF and Chrome. See https://usercontent.irccloud-cdn.com/file/xvnTjWYD/Screen%20Shot%202017-08-28...
https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29502647/diff/29524621/skin/new-options.cs... skin/new-options.css:60: font-size: 1rem; On 2017/08/28 11:16:41, juliandoucette wrote: > On 2017/08/23 18:11:08, juliandoucette wrote: > > Yes, but you set the font-size on the html element which overrides the > browsers > > font-size property and makes this change inaccessible. I suggest that you > unset > > the font-size on the HTML element and re-calculate REM values accordingly if > you > > choose to use REMs. > > NIT: This is still an issue. Yes it is, this was discussed multiple times. This will require design change as well. https://codereview.adblockplus.org/29502647/diff/29528555/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29502647/diff/29528555/new-options.html#ne... new-options.html:38: Adblock <strong>Plus</strong> On 2017/08/28 11:16:41, juliandoucette wrote: > NIT: The weight of strong is different in FF and Chrome. See > https://usercontent.irccloud-cdn.com/file/xvnTjWYD/Screen%20Shot%202017-08-28... Fixed.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
