Issue 29601581: Issue 5872 - apply style changes (Closed)
DescriptionIssue 5872 - apply style changes
Patch Set 1 : #
Total comments: 38
Patch Set 2 : Addressed Ire's comments #
Total comments: 27
Patch Set 3 : Additional style fixes #
Total comments: 2
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
MessagesTotal messages: 10
@Ire can you please have a look. If you will test in the Chrome please note there is another issue -> https://issues.adblockplus.org/ticket/5990 Regarding the fragment identifiers usage is being broken in current dev env for Chrome. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:1173: font-size: 1rem; Note: There is a difference between font-sizes for buttons and <a> tags in this context menu, I'm not sure if there is an easy fix? This needs an investigation, if it's not easy fixable make sense to tackle separately.
On 2017/11/08 15:46:46, saroyanm wrote: > @Ire can you please have a look. > > If you will test in the Chrome please note there is another issue -> > https://issues.adblockplus.org/ticket/5990 > Regarding the fragment identifiers usage is being broken in current dev env for > Chrome. Thanks for letting me know. I've had my first look. I don't think I was able to get to absolutely everything, but I will take another look after you've addressed some of the comments. https://codereview.adblockplus.org/29601581/diff/29601610/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29601581/diff/29601610/desktop-options.htm... desktop-options.html:228: <button data-action="toggle-disable-subscription" role="checkbox" class="control toggle"> Wasn't sure where else to put this comment, but this button is no longer aligned vertically centered with the text beside it. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:52: body The line-height isn't set at all? It's supposed to be 1.5 https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:62: color: #494949; According to the spec this colour should be #4A4A4A? https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:80: text-decoration: none; Shouldn't this be underlined? https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:113: .button Since Chrome 62, buttons by default have rounded corners so you would need to set `border-radius: 0` https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:202: width: 1.7rem; This should be 1.9rem according to the spec (https://bitbucket.org/adblockplus/spec/src/b140349a623ca03a314b03009cecbff948...) https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:246: border: 0.2rem solid transparent; I don't understand the purpose of the border, could you please explain? I also discovered that it was causing extra spacing in the .context-menu icon https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:556: width: 46.3rem; I think your intention was to force this element to always be 46.3rem and never resize, but it still does. If that was your intention, you also need to add `min-width: 46.3rem` https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:708: margin: 0rem 0rem 0.8rem 0rem; I don't think you need to apply margins to all of the sides here. `margin-bottom: 0.8rem` works the same. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:723: padding: 0.5rem 1.0rem; You don't need the .0 here. i.e 1rem is enough https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:794: flex: 7; NIT: It's not exactly clear how these flex numbers came about. Suggestions: 1. The last two columns should be the same size (`flex: 1`) 2. The first and fourth columns should be 4 times the base size (`flex: 4`) 3. The second column, like you did, is double the first column (`flex: 8`) https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:827: font-size: 1rem; I don't think setting the font-size is necessary here, the body font-size is inherited https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:966: #content-whitelist form input The font-family for this input isn't the "Source Sans Pro", it's the default system font https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:1173: font-size: 1rem; On 2017/11/08 15:46:46, saroyanm wrote: > Note: There is a difference between font-sizes for buttons and <a> tags in this > context menu, I'm not sure if there is an easy fix? > This needs an investigation, if it's not easy fixable make sense to tackle > separately. It's because the font families aren't actually the same. I think you need to set `font-family: "Source Sans Pro"` here
Thanks Ire Sorry that it took so long to address your comments. Just got back from the vacation. https://codereview.adblockplus.org/29601581/diff/29601610/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29601581/diff/29601610/desktop-options.htm... desktop-options.html:228: <button data-action="toggle-disable-subscription" role="checkbox" class="control toggle"> On 2017/11/09 09:08:28, ire wrote: > Wasn't sure where else to put this comment, but this button is no longer aligned > vertically centered with the text beside it. Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:52: body On 2017/11/09 09:08:29, ire wrote: > The line-height isn't set at all? It's supposed to be 1.5 Yes, it's not, the style guide are being constantly updated. Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:62: color: #494949; On 2017/11/09 09:08:28, ire wrote: > According to the spec this colour should be #4A4A4A? Well spotted, done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:80: text-decoration: none; On 2017/11/09 09:08:28, ire wrote: > Shouldn't this be underlined? Well spotted, seems like I missed when this was updated. Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:113: .button On 2017/11/09 09:08:28, ire wrote: > Since Chrome 62, buttons by default have rounded corners so you would need to > set `border-radius: 0` Interesting, didn't know about that, apparently that affects only Mac users. Updated. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:202: width: 1.7rem; On 2017/11/09 09:08:29, ire wrote: > This should be 1.9rem according to the spec > (https://bitbucket.org/adblockplus/spec/src/b140349a623ca03a314b03009cecbff948...) You are right, the SVG should have been updated first, because in Firefox it was clear that proportions was wrong, now it should be right dimensions. Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:246: border: 0.2rem solid transparent; On 2017/11/09 09:08:28, ire wrote: > I don't understand the purpose of the border, could you please explain? > > I also discovered that it was causing extra spacing in the .context-menu icon I think I should have left a comment here: There was a question whether the 1rem is small enough or not, Jeen asked me to leave the icons size 1rem, but increase the hover(click) area, see -> https://bitbucket.org/adblockplus/spec/pull-requests/84/issue-89-adjusted-fon... It mentioned Padding there, but intention is to have broader clickable zone. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:556: width: 46.3rem; On 2017/11/09 09:08:29, ire wrote: > I think your intention was to force this element to always be 46.3rem and never > resize, but it still does. If that was your intention, you also need to add > `min-width: 46.3rem` My intention was to prevent a main area resize effect, switching from/to Whitelisting website were causing an annoying resize effect https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:708: margin: 0rem 0rem 0.8rem 0rem; On 2017/11/09 09:08:28, ire wrote: > I don't think you need to apply margins to all of the sides here. > `margin-bottom: 0.8rem` works the same. Agree, done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:723: padding: 0.5rem 1.0rem; On 2017/11/09 09:08:28, ire wrote: > You don't need the .0 here. i.e 1rem is enough Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:794: flex: 7; On 2017/11/09 09:08:28, ire wrote: > NIT: It's not exactly clear how these flex numbers came about. Mostly eyepick > Suggestions: > > 1. The last two columns should be the same size (`flex: 1`) > 2. The first and fourth columns should be 4 times the base size (`flex: 4`) > 3. The second column, like you did, is double the first column (`flex: 8`) I agree with you. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:827: font-size: 1rem; On 2017/11/09 09:08:29, ire wrote: > I don't think setting the font-size is necessary here, the body font-size is > inherited Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:966: #content-whitelist form input On 2017/11/09 09:08:29, ire wrote: > The font-family for this input isn't the "Source Sans Pro", it's the default > system font Well spotted! Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:1173: font-size: 1rem; On 2017/11/09 09:08:29, ire wrote: > On 2017/11/08 15:46:46, saroyanm wrote: > > Note: There is a difference between font-sizes for buttons and <a> tags in > this > > context menu, I'm not sure if there is an easy fix? > > This needs an investigation, if it's not easy fixable make sense to tackle > > separately. > > It's because the font families aren't actually the same. I think you need to set > `font-family: "Source Sans Pro"` here Right, Done :)
On 2017/11/13 17:07:15, saroyanm wrote: > Thanks Ire > Sorry that it took so long to address your comments. > Just got back from the vacation. No problem, hope you enjoyed your holiday :) I've gone over everything in more details so I think these should be my final comments. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:246: border: 0.2rem solid transparent; On 2017/11/13 17:07:15, saroyanm wrote: > On 2017/11/09 09:08:28, ire wrote: > > I don't understand the purpose of the border, could you please explain? > > > > I also discovered that it was causing extra spacing in the .context-menu icon > > I think I should have left a comment here: > There was a question whether the 1rem is small enough or not, Jeen asked me to > leave the icons size 1rem, but increase the hover(click) area, see -> > https://bitbucket.org/adblockplus/spec/pull-requests/84/issue-89-adjusted-fon... > > It mentioned Padding there, but intention is to have broader clickable zone. Okay I understand now, thanks! The issue in the .context-menu still exists though. Perhaps you should add this border only to the icon in the table, so it doesn't interfere with that other use case? https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:257: button.link This should be underlined to look like the <a> links https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:522: margin: 1.2rem 0rem; The vertical spacing here should be 1rem https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:556: width: 46.3rem; On 2017/11/13 17:07:15, saroyanm wrote: > On 2017/11/09 09:08:29, ire wrote: > > I think your intention was to force this element to always be 46.3rem and > never > > resize, but it still does. If that was your intention, you also need to add > > `min-width: 46.3rem` > > My intention was to prevent a main area resize effect, switching from/to > Whitelisting website were causing an annoying resize effect Acknowledged. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:577: padding: 2rem; According to the spec this spacing should be 1.4rem https://codereview.adblockplus.org/29601581/diff/29606580/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29601581/diff/29606580/desktop-options.htm... desktop-options.html:324: <li><a id="twitter" target="_blank">Twitter</a></li> These links are now also underlined. It's not clear from the spec if they should be or not https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:115: font-family: "Source Sans Pro", sans-serif; Suggest: You can set the font-family to `inherit` so you don't have to write out the full family multiple times https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:277: font-weight: 300; This font weight should be the same as the body, which is 400 https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:300: box-shadow: inset 0 0 0 3px #077CA6; NIT: Because there's no top border, the box-shadow looks uneven (it looks less thick on the top than the other sides) https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:353: font-size: 1.3rem; According to the spec this should now be 1rem (although that seems a bit small to me) This may also be the case for the actual input text but I can't find it written anywhere in the spec https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:739: padding: 0.5rem 1rem; The horizontal padding on the default tables should be 1.4rem. This 1rem applies to only the complex tables https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:766: font-weight: 700; This should be styled like the body text, i.e. 400 font weight https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1067: padding: 1.3rem; According to the spec this padding should be 1rem https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1185: font-weight: 700; It looks like this should be 400 in the spec images, but it's not specified anywhere so I'm not sure https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1320: #dialog header h3 It's a bit unclear from the spec how this should look because it looks one way in the image, but the style name says something different. I assume your implementation is the correct way, but just bringing this up in case it's not. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1385: #abp-version I don't think this style is needed?
Thanks for the comments, new patch is uploaded and ready for the review. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:246: border: 0.2rem solid transparent; On 2017/11/14 16:55:20, ire wrote: > On 2017/11/13 17:07:15, saroyanm wrote: > > On 2017/11/09 09:08:28, ire wrote: > > > I don't understand the purpose of the border, could you please explain? > > > > > > I also discovered that it was causing extra spacing in the .context-menu > icon > > > > I think I should have left a comment here: > > There was a question whether the 1rem is small enough or not, Jeen asked me to > > leave the icons size 1rem, but increase the hover(click) area, see -> > > > https://bitbucket.org/adblockplus/spec/pull-requests/84/issue-89-adjusted-fon... > > > > It mentioned Padding there, but intention is to have broader clickable zone. > > Okay I understand now, thanks! > > The issue in the .context-menu still exists though. Perhaps you should add this > border only to the icon in the table, so it doesn't interfere with that other > use case? Right, I missed the part about the .context-menu .context-menu is also in the table and we are using delete icons in different tables as well, I think we should have a rule for context-menu which will override this. Done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:257: button.link On 2017/11/14 16:55:21, ire wrote: > This should be underlined to look like the <a> links I agree, done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:522: margin: 1.2rem 0rem; On 2017/11/14 16:55:21, ire wrote: > The vertical spacing here should be 1rem Well spotted, done. https://codereview.adblockplus.org/29601581/diff/29601610/skin/desktop-option... skin/desktop-options.css:577: padding: 2rem; On 2017/11/14 16:55:21, ire wrote: > According to the spec this spacing should be 1.4rem You are right, vertical spacings seems to be specified as 1.4rem, but horizontal looks to still be 2rem. Done. https://codereview.adblockplus.org/29601581/diff/29606580/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29601581/diff/29606580/desktop-options.htm... desktop-options.html:324: <li><a id="twitter" target="_blank">Twitter</a></li> On 2017/11/14 16:55:21, ire wrote: > These links are now also underlined. It's not clear from the spec if they should > be or not Yes, I've removed them, will introduce them back if Jeen will say so. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:115: font-family: "Source Sans Pro", sans-serif; On 2017/11/14 16:55:22, ire wrote: > Suggest: You can set the font-family to `inherit` so you don't have to write out > the full family multiple times Done. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:277: font-weight: 300; On 2017/11/14 16:55:22, ire wrote: > This font weight should be the same as the body, which is 400 Done. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:300: box-shadow: inset 0 0 0 3px #077CA6; On 2017/11/14 16:55:21, ire wrote: > NIT: Because there's no top border, the box-shadow looks uneven (it looks less > thick on the top than the other sides) I see, not sure what will be quick fix for this, if I'll add a border on hover it will move content down which will be more annoying. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:353: font-size: 1.3rem; On 2017/11/14 16:55:22, ire wrote: > According to the spec this should now be 1rem (although that seems a bit small > to me) Done. > This may also be the case for the actual input text but I can't find it written > anywhere in the spec I think it should same as placeholder text when it's not moved on top of the field, which is specified to be 1rem. done. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:739: padding: 0.5rem 1rem; On 2017/11/14 16:55:21, ire wrote: > The horizontal padding on the default tables should be 1.4rem. This 1rem applies > to only the complex tables Seems like it's 1rem still ( https://bitbucket.org/adblockplus/spec/src/b140349a623ca03a314b03009cecbff948... ), The exception looks to be only for the table in the Whitelisted tab. Done. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:766: font-weight: 700; On 2017/11/14 16:55:22, ire wrote: > This should be styled like the body text, i.e. 400 font weight Done. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1067: padding: 1.3rem; On 2017/11/14 16:55:21, ire wrote: > According to the spec this padding should be 1rem Done, I also didn't calculate the vertical margins of the padding inside. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1185: font-weight: 700; On 2017/11/14 16:55:21, ire wrote: > It looks like this should be 400 in the spec images, but it's not specified > anywhere so I'm not sure Agree, done. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1320: #dialog header h3 On 2017/11/14 16:55:22, ire wrote: > It's a bit unclear from the spec how this should look because it looks one way > in the image, but the style name says something different. I assume your > implementation is the correct way, but just bringing this up in case it's not. Thanks for bringing this up, the agreement is to prioritize text over the visuals. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1385: #abp-version On 2017/11/14 16:55:22, ire wrote: > I don't think this style is needed? Done.
https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:300: box-shadow: inset 0 0 0 3px #077CA6; On 2017/11/14 19:19:01, saroyanm wrote: > On 2017/11/14 16:55:21, ire wrote: > > NIT: Because there's no top border, the box-shadow looks uneven (it looks less > > thick on the top than the other sides) > > I see, not sure what will be quick fix for this, if I'll add a border on hover > it will move content down which will be more annoying. I think you can have the border on all sides normally, and remove the border-bottom on .table li:last-of-type https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1197: .context-menu li[role="menuitem"] > *::before The icons are not vertically aligned with the text https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1320: #dialog header h3 On 2017/11/14 19:19:00, saroyanm wrote: > On 2017/11/14 16:55:22, ire wrote: > > It's a bit unclear from the spec how this should look because it looks one way > > in the image, but the style name says something different. I assume your > > implementation is the correct way, but just bringing this up in case it's not. > > Thanks for bringing this up, the agreement is to prioritize text over the > visuals. Acknowledged. https://codereview.adblockplus.org/29601581/diff/29608584/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29601581/diff/29608584/desktop-options.htm... desktop-options.html:130: <button data-single="visible" data-action="open-dialog" data-dialog="language-change" class="i18n_options_language_change link"></button> This probably shouldn't be underlined
Thanks Ire, New patch is uploaded and ready for the review. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:300: box-shadow: inset 0 0 0 3px #077CA6; On 2017/11/15 07:00:46, ire wrote: > On 2017/11/14 19:19:01, saroyanm wrote: > > On 2017/11/14 16:55:21, ire wrote: > > > NIT: Because there's no top border, the box-shadow looks uneven (it looks > less > > > thick on the top than the other sides) > > > > I see, not sure what will be quick fix for this, if I'll add a border on hover > > it will move content down which will be more annoying. > > I think you can have the border on all sides normally, and remove the > border-bottom on .table li:last-of-type That will require to treat this lists differently, but apparently it's unavoidable. Done. https://codereview.adblockplus.org/29601581/diff/29606580/skin/desktop-option... skin/desktop-options.css:1197: .context-menu li[role="menuitem"] > *::before On 2017/11/15 07:00:46, ire wrote: > The icons are not vertically aligned with the text Done. https://codereview.adblockplus.org/29601581/diff/29608584/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29601581/diff/29608584/desktop-options.htm... desktop-options.html:130: <button data-single="visible" data-action="open-dialog" data-dialog="language-change" class="i18n_options_language_change link"></button> On 2017/11/15 07:00:46, ire wrote: > This probably shouldn't be underlined Done.
Thanks! A couple more comments but otherwise LGTM https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:59: margin: 1.2rem 0.3rem; The vertical margin should be 1rem https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:379: [data-validation] .floating-input input:valid ~ .attention::before This element is too close to the input underline. There should be more space between the two https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:764: padding: 1.3em 1.9em; This padding should be the same as `padding: 0.5rem 1rem;` https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:907: #dialog .table.list li button .display This element is currently in bold weight since it's a button, but I think it should be same as body font weight
Thanks, addressed last comments, ready for the review https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... File skin/desktop-options.css (right): https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:59: margin: 1.2rem 0.3rem; On 2017/11/15 13:49:28, ire wrote: > The vertical margin should be 1rem Done. https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:379: [data-validation] .floating-input input:valid ~ .attention::before On 2017/11/15 13:49:28, ire wrote: > This element is too close to the input underline. There should be more space > between the two Done. https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:764: padding: 1.3em 1.9em; On 2017/11/15 13:49:28, ire wrote: > This padding should be the same as `padding: 0.5rem 1rem;` According to specs, it's 1rem 1.4rem, I've updated accordingly ( https://bitbucket.org/adblockplus/spec/src/b140349a623ca03a314b03009cecbff948... ) https://codereview.adblockplus.org/29601581/diff/29609564/skin/desktop-option... skin/desktop-options.css:907: #dialog .table.list li button .display On 2017/11/15 13:49:28, ire wrote: > This element is currently in bold weight since it's a button, but I think it > should be same as body font weight Right, done.
On 2017/11/15 14:51:23, saroyanm wrote: > Thanks, addressed last comments, > ready for the review Thanks LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
