Issue 29336364: issue 2377 - Finish design of Advanced tab of new options page (Closed)
|
Created:
Feb. 15, 2016, 12:29 p.m. by saroyanm Modified:
April 29, 2016, 9:34 a.m. Visibility:
Public. |
Descriptionissue 2377 - Finish design of Advanced tab of new options page
Patch Set 1 #
Total comments: 34
Patch Set 2 : Addressed Thomas and Wladimir comments #Patch Set 3 : Rebase to changeset 72 #
Total comments: 49
Patch Set 4 : Addressed Thomas comments #
Total comments: 2
Patch Set 5 : Rebased to changeset #78 #Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
MessagesTotal messages: 16
https://codereview.adblockplus.org/29336364/diff/29336365/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29336364/diff/29336365/locale/en-US/option... locale/en-US/options.json:250: "options_customFilter_list": { Detail: You changed the meaning of the string so please also adapt the message ID and description to reflect the new string. https://codereview.adblockplus.org/29336364/diff/29336365/options.html File options.html (right): https://codereview.adblockplus.org/29336364/diff/29336365/options.html#newcod... options.html:321: <span class="i18n_options_customFilter_save"></span> Detail: You're using "icon-cancel" and "i18n_options_customFilter_save" here. The same also applies to the button below. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:464: width: 18px; Those two icons are 10x10 pixels so let's not have unnecessary whitespace in the sprite image. Also note that this makes it much more difficult to precisely specify how many pixels of whitespace should be between the icon and text or other elements. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:472: width: 18px; Would be nice if we could remove all those duplicated styles if it's not too much effort for this code review. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:506: position: relative; Detail: I know that you merely moved this but I'm wondering why it was added (see r15) because it seems redundant. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:510: -webkit-padding-start: 16px; Detail: According to the style guide there should be `16px` padding both before and after the "save" and "cancel" buttons. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:528: .controls button span:not(.icon) Detail: The button text is smaller than `14px`. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:628: h3.with-description Why is this necessary? Seems like you should be able to achieve the same effect by setting `display: inline-block` on the "read more" `<div>` https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:636: margin-top: 18px; It seems like this margin is the same for each `h1` tag so please apply this style more generically. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:642: padding-bottom: 6px; Can't you make this a bit more generic? I noticed that you're using `table.list` for most of the tables to indicate that they have a zebra pattern so why not use `table:not(.list) li` here? https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:685: } Detail: Wouldn't it be simpler to replace the `<span>` with a `<strong>` tag? In that case you wouldn't need to introduce the "own-filter-list" ID either. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:692: #filter-lists h3 I noticed that we decided to move the "read more" text to the top-right corner of the section so I'm wondering whether we should also do that for the "read more" texts in the Advanced tab. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:695: font-size: 16px; Detail: Why not make this apply globally for all `<h3>` tags? https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:789: display: flex; Detail: This seems redundant because `.controls > div` already defines it. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:825: -webkit-padding-start: 16px; Detail: Any reason why you're only setting the padding on one side but not the other? Same question for the change below.
https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:878: -moz-margin-start: -60px; This apparently assumes a particular text size. Opening that page in Firefox or replacing "add" by slightly longer text (translations!) will make this wrap to two lines and look very ugly.
https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:845: padding-bottom: 18px; This makes no sense, it's impossible to have height 25px and 18px padding at top and bottom at the same time. Chrome does something weird in order to display this text field nevertheless, in Firefox the text isn't visible. If you remove the height property and change padding-top and padding-bottom to 10px the display doesn't seem to change on Chrome, Firefox however will produce the same results then.
https://codereview.adblockplus.org/29336364/diff/29336365/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29336364/diff/29336365/locale/en-US/option... locale/en-US/options.json:250: "options_customFilter_list": { On 2016/02/15 18:09:21, Thomas Greiner wrote: > Detail: You changed the meaning of the string so please also adapt the message > ID and description to reflect the new string. Done. https://codereview.adblockplus.org/29336364/diff/29336365/options.html File options.html (right): https://codereview.adblockplus.org/29336364/diff/29336365/options.html#newcod... options.html:321: <span class="i18n_options_customFilter_save"></span> On 2016/02/15 18:09:21, Thomas Greiner wrote: > Detail: You're using "icon-cancel" and "i18n_options_customFilter_save" here. > > The same also applies to the button below. Oops, thanks. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:464: width: 18px; On 2016/02/15 18:09:22, Thomas Greiner wrote: > Those two icons are 10x10 pixels so let's not have unnecessary whitespace in the > sprite image. Also note that this makes it much more difficult to precisely > specify how many pixels of whitespace should be between the icon and text or > other elements. Fair enough, done. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:472: width: 18px; On 2016/02/15 18:09:21, Thomas Greiner wrote: > Would be nice if we could remove all those duplicated styles if it's not too > much effort for this code review. Sure, hopefully I could organize the code better. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:506: position: relative; On 2016/02/15 18:09:23, Thomas Greiner wrote: > Detail: I know that you merely moved this but I'm wondering why it was added > (see r15) because it seems redundant. Yes, neither can't see a reason. Done. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:510: -webkit-padding-start: 16px; On 2016/02/15 18:09:22, Thomas Greiner wrote: > Detail: According to the style guide there should be `16px` padding both before > and after the "save" and "cancel" buttons. Done, I also changed the implementation so we don't need to set the border-top to the control element, while it's something related to the table itself. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:528: .controls button span:not(.icon) On 2016/02/15 18:09:22, Thomas Greiner wrote: > Detail: The button text is smaller than `14px`. Not really sure, according to styleguide -> https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... The readmore text visually looks to be smaller than the text next to the button, or I can imagine that the text next to the button is 12px but bold, IMO we can leave this for later, for design polish stage. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:628: h3.with-description On 2016/02/15 18:09:21, Thomas Greiner wrote: > Why is this necessary? Seems like you should be able to achieve the same effect > by setting `display: inline-block` on the "read more" `<div>` I did it for the reason of the RTL positioning in case of RTL languages, but apparently with inline-block; it also works fine, fare enough. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:636: margin-top: 18px; On 2016/02/15 18:09:22, Thomas Greiner wrote: > It seems like this margin is the same for each `h1` tag so please apply this > style more generically. Done, but not sure does it make sense to remove padding-top for the horizontal tabs ? Because if I remove it also affects usability more. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:642: padding-bottom: 6px; On 2016/02/15 18:09:22, Thomas Greiner wrote: > Can't you make this a bit more generic? I noticed that you're using `table.list` > for most of the tables to indicate that they have a zebra pattern so why not use > `table:not(.list) li` here? Done. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:685: } On 2016/02/15 18:09:22, Thomas Greiner wrote: > Detail: Wouldn't it be simpler to replace the `<span>` with a `<strong>` tag? In > that case you wouldn't need to introduce the "own-filter-list" ID either. Makes much more sense, done, I removed the ID, but I assume that I'll introduce it again with the functionality of User filter list switcher. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:692: #filter-lists h3 On 2016/02/15 18:09:22, Thomas Greiner wrote: > I noticed that we decided to move the "read more" text to the top-right corner > of the section so I'm wondering whether we should also do that for the "read > more" texts in the Advanced tab. Well there we do have two section next to each other while here the "read more" link will be too far away from the related element. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:695: font-size: 16px; On 2016/02/15 18:09:22, Thomas Greiner wrote: > Detail: Why not make this apply globally for all `<h3>` tags? My fault, according to the styleguid, we do not have h3, but we are using relatively smaller size for the h3 in the dialog windows, so I changed the to h2 here. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:789: display: flex; On 2016/02/15 18:09:22, Thomas Greiner wrote: > Detail: This seems redundant because `.controls > div` already defines it. Done. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:825: -webkit-padding-start: 16px; On 2016/02/15 18:09:22, Thomas Greiner wrote: > Detail: Any reason why you're only setting the padding on one side but not the > other? > > Same question for the change below. Done. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:845: padding-bottom: 18px; On 2016/02/23 15:26:04, Wladimir Palant wrote: > This makes no sense, it's impossible to have height 25px and 18px padding at top > and bottom at the same time. Chrome does something weird in order to display > this text field nevertheless, in Firefox the text isn't visible. > > If you remove the height property and change padding-top and padding-bottom to > 10px the display doesn't seem to change on Chrome, Firefox however will produce > the same results then. Yes you are right, but the issue is not about the browser doing something weird, but the implementation being wrong, we are using "border-box" so the height is calculated including the padding, if we will remove height, then we will rely on browser to set the height, which will work in most cases, alternatively we could explicit set height which will include font-size+border-bottom+bottom-top+borders which doesn't make sense while it will rely on other properties, so I agree with you that in this case it's better to rely on browser. https://codereview.adblockplus.org/29336364/diff/29336365/skin/options.css#ne... skin/options.css:878: -moz-margin-start: -60px; On 2016/02/23 15:16:08, Wladimir Palant wrote: > This apparently assumes a particular text size. Opening that page in Firefox or > replacing "add" by slightly longer text (translations!) will make this wrap to > two lines and look very ugly. Thanks for noticing this, totally forgot about this, the implementation is wrong here. Fixed.
Would you mind rebasing it? Then I can take another look at it so that we can get this review closed.
On 2016/04/07 17:33:13, Thomas Greiner wrote: > Would you mind rebasing it? Then I can take another look at it so that we can > get this review closed. Done.
https://codereview.adblockplus.org/29336364/diff/29339606/options.html File options.html (right): https://codereview.adblockplus.org/29336364/diff/29339606/options.html#newcod... options.html:247: <h1 class="with-description"> Why do you need this class? All it seems to be used for is to set `display: inline-block` on `.tooltip` elements. https://codereview.adblockplus.org/29336364/diff/29339606/options.html#newcod... options.html:293: <strong class="i18n_options_filterList_own_list"></strong> Detail: For accessibility reasons, I'd suggest that this should remain a `<label>` and only be formatted via CSS. Note that the connection between checkbox and label is missing here so adding the `for="..."` attribute would fix that. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (left): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ol... skin/options.css:620: #restart-safari Why did you remove that? https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ol... skin/options.css:674: font-size: 14px; Regression: Removing this causes the font size to be `16px`. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:439: border-bottom: 1px solid #A1A1A1; Detail: Should this be #CDCDCD instead? see https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:470: height: 18px; According to https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... those icons should be 16x16. Note that there's a 1px space around each icon in the sprite so you may need to adapt the background position of the icons as well. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:496: height: 10px; Detail: Based on the sprite it looks like this should be 8x8. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:516: height: 12px; Detail: Based on the sprite it looks like this should be 8x8. Note that Sebastian already fixed the dialog close button in https://hg.adblockplus.org/adblockplusui/rev/ca3ff1974808 https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:537: margin-right: 16px; Regression: The "+" icon below the "Whitelisted websites" section is no longer aligned with the "x" buttons of the list. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:556: padding: 0; Coding style: "CSS number values should specify units where possible." https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:567: padding-top: 1px; Detail: Why is that necessary? I know that you introduced this change here but I'm still wondering why this is `display: inline-block` and has `padding-top: 1px` because it seems a bit unnecessary. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:649: position: absolute; Regression: The "enter" icon is no longer in the correct position. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:834: font-size: 13px; Detail: There's no mention of `13px` in the style guide at https://issues.adblockplus.org/attachment/ticket/1526/options%20page_stylegui.... https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:836: flex-grow: 1; Detail: That's the same as `width: 100%` or not? https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:847: font-weight: 600; Detail: That doesn't appear to work. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:852: height: 290px; Detail: Shouldn't this be `240px`? (i.e. 6 * 40px) https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:853: width: auto; Regression: This will cause longer filters to expand the width of the filter input box and by that also the rest of the page. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:865: flex-grow: 0; Detail: Why is this necessary? Removing it doesn't seem to change anything. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:927: #block-element-explanation a This is not matching any elements.
Thanks Thomas for reviewing, addressed your comments and rebased. https://codereview.adblockplus.org/29336364/diff/29339606/options.html File options.html (right): https://codereview.adblockplus.org/29336364/diff/29339606/options.html#newcod... options.html:247: <h1 class="with-description"> On 2016/04/18 18:32:51, Thomas Greiner wrote: > Why do you need this class? All it seems to be used for is to set `display: > inline-block` on `.tooltip` elements. I used to align the content inside inline, for read-more links, but it was stupid implementation, so made it more generic, thanks. https://codereview.adblockplus.org/29336364/diff/29339606/options.html#newcod... options.html:293: <strong class="i18n_options_filterList_own_list"></strong> On 2016/04/18 18:32:51, Thomas Greiner wrote: > Detail: For accessibility reasons, I'd suggest that this should remain a > `<label>` and only be formatted via CSS. > > Note that the connection between checkbox and label is missing here so adding > the `for="..."` attribute would fix that. Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (left): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ol... skin/options.css:620: #restart-safari On 2016/04/18 18:32:53, Thomas Greiner wrote: > Why did you remove that? I think by accident, restored. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ol... skin/options.css:674: font-size: 14px; On 2016/04/18 18:32:53, Thomas Greiner wrote: > Regression: Removing this causes the font size to be `16px`. Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:439: border-bottom: 1px solid #A1A1A1; On 2016/04/18 18:32:53, Thomas Greiner wrote: > Detail: Should this be #CDCDCD instead? > > see > https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... true, Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:470: height: 18px; On 2016/04/18 18:32:53, Thomas Greiner wrote: > According to > https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... > those icons should be 16x16. Note that there's a 1px space around each icon in > the sprite so you may need to adapt the background position of the icons as > well. Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:496: height: 10px; On 2016/04/18 18:32:53, Thomas Greiner wrote: > Detail: Based on the sprite it looks like this should be 8x8. Yes you are right, part of them. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:516: height: 12px; On 2016/04/18 18:32:53, Thomas Greiner wrote: > Detail: Based on the sprite it looks like this should be 8x8. Note that > Sebastian already fixed the dialog close button in > https://hg.adblockplus.org/adblockplusui/rev/ca3ff1974808 Date and time looks to be 12px. So I think the only problem is with dialog-close, I think after rebasing this should be fixed in that case. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:537: margin-right: 16px; On 2016/04/18 18:32:53, Thomas Greiner wrote: > Regression: The "+" icon below the "Whitelisted websites" section is no longer > aligned with the "x" buttons of the list. Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:556: padding: 0; On 2016/04/18 18:32:52, Thomas Greiner wrote: > Coding style: "CSS number values should specify units where possible." Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:567: padding-top: 1px; On 2016/04/18 18:32:53, Thomas Greiner wrote: > Detail: Why is that necessary? > > I know that you introduced this change here but I'm still wondering why this is > `display: inline-block` and has `padding-top: 1px` because it seems a bit > unnecessary. Seems like it was here before, I just rearranged to match the coding style, but I also can't see reason why we had this here in first place, so removed, thanks. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:649: position: absolute; On 2016/04/18 18:32:52, Thomas Greiner wrote: > Regression: The "enter" icon is no longer in the correct position. Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:834: font-size: 13px; On 2016/04/18 18:32:53, Thomas Greiner wrote: > Detail: There's no mention of `13px` in the style guide at > https://issues.adblockplus.org/attachment/ticket/1526/options%20page_stylegui.... Not sure if we have Style guide for this element, could only find this: https://issues.adblockplus.org/attachment/ticket/1526/advanced_2_77.png Anyway removed completely, doesn't add too much value. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:836: flex-grow: 1; On 2016/04/18 18:32:51, Thomas Greiner wrote: > Detail: That's the same as `width: 100%` or not? Yes the effect in this case will be same, but I think maybe we should use flexbox related properties where possible ? https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:847: font-weight: 600; On 2016/04/18 18:32:52, Thomas Greiner wrote: > Detail: That doesn't appear to work. In the end it's not standardized, so I think we can remove it, removed. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:852: height: 290px; On 2016/04/18 18:32:52, Thomas Greiner wrote: > Detail: Shouldn't this be `240px`? (i.e. 6 * 40px) Why you think this should be 240px ? I make it so to relatively be of same height as the textarea when swithching the views. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:853: width: auto; On 2016/04/18 18:32:51, Thomas Greiner wrote: > Regression: This will cause longer filters to expand the width of the filter > input box and by that also the rest of the page. We are using overflow: hidden, but while it's the initial value of the item removing this doesn't affect anything. But not sure if the behavior should be changed ? https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:865: flex-grow: 0; On 2016/04/18 18:32:52, Thomas Greiner wrote: > Detail: Why is this necessary? Removing it doesn't seem to change anything. True not necessary, it's initial value is 0, so should be fine. thanks. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:927: #block-element-explanation a On 2016/04/18 18:32:54, Thomas Greiner wrote: > This is not matching any elements. true, removed.
Just some smaller details left. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:516: height: 12px; On 2016/04/20 14:29:00, saroyanm wrote: > On 2016/04/18 18:32:53, Thomas Greiner wrote: > > Detail: Based on the sprite it looks like this should be 8x8. Note that > > Sebastian already fixed the dialog close button in > > https://hg.adblockplus.org/adblockplusui/rev/ca3ff1974808 > > Date and time looks to be 12px. > So I think the only problem is with dialog-close, I think after rebasing this > should be fixed in that case. Correct, it doesn't apply to the "date" and "time" icons but I just noticed that the "date" icon is 11x12 and not 12x12 like the "time" icon. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:834: font-size: 13px; On 2016/04/20 14:29:00, saroyanm wrote: > On 2016/04/18 18:32:53, Thomas Greiner wrote: > > Detail: There's no mention of `13px` in the style guide at > > > https://issues.adblockplus.org/attachment/ticket/1526/options%20page_stylegui.... > > Not sure if we have Style guide for this element, could only find this: > https://issues.adblockplus.org/attachment/ticket/1526/advanced_2_77.png > Anyway removed completely, doesn't add too much value. It's not specifically mentioned but it mentions the allowed font styles at https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... which don't include any 13px-styles. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:836: flex-grow: 1; On 2016/04/20 14:29:00, saroyanm wrote: > On 2016/04/18 18:32:51, Thomas Greiner wrote: > > Detail: That's the same as `width: 100%` or not? > > Yes the effect in this case will be same, but I think maybe we should use > flexbox related properties where possible ? I'm torn on this one but I'd say we should minimize the use of flexbox to make it easier to support Safari. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:852: height: 290px; On 2016/04/20 14:28:59, saroyanm wrote: > On 2016/04/18 18:32:52, Thomas Greiner wrote: > > Detail: Shouldn't this be `240px`? (i.e. 6 * 40px) > > Why you think this should be 240px ? I make it so to relatively be of same > height as the textarea when swithching the views. It's based on two pieces of information from the style guide: - The height of a list element is `40px` (see https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide...) - Six elements should be visible in the custom filters list (see https://issues.adblockplus.org/attachment/ticket/1526/advanced_2_77.png) https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:853: width: auto; On 2016/04/20 14:28:59, saroyanm wrote: > On 2016/04/18 18:32:51, Thomas Greiner wrote: > > Regression: This will cause longer filters to expand the width of the filter > > input box and by that also the rest of the page. > > We are using overflow: hidden, but while it's the initial value of the item > removing this doesn't affect anything. > But not sure if the behavior should be changed ? `overflow: hidden` doesn't do anything if you don't specify the width. So what we should do is specify the width instead of setting it to `auto`. https://codereview.adblockplus.org/29336364/diff/29340648/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29340648/skin/options.css#ne... skin/options.css:316: #content h2 > * Detail: We should avoid the star selector whenever possible so why not just add it to the existing `.tooltip` rule?
https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:516: height: 12px; On 2016/04/22 16:36:23, Thomas Greiner wrote: > On 2016/04/20 14:29:00, saroyanm wrote: > > On 2016/04/18 18:32:53, Thomas Greiner wrote: > > > Detail: Based on the sprite it looks like this should be 8x8. Note that > > > Sebastian already fixed the dialog close button in > > > https://hg.adblockplus.org/adblockplusui/rev/ca3ff1974808 > > > > Date and time looks to be 12px. > > So I think the only problem is with dialog-close, I think after rebasing this > > should be fixed in that case. > > Correct, it doesn't apply to the "date" and "time" icons but I just noticed that > the "date" icon is 11x12 and not 12x12 like the "time" icon. True, but doesn't make sense I assume to separate them in this case, maybe in future we will update the Icon. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:836: flex-grow: 1; On 2016/04/22 16:36:24, Thomas Greiner wrote: > On 2016/04/20 14:29:00, saroyanm wrote: > > On 2016/04/18 18:32:51, Thomas Greiner wrote: > > > Detail: That's the same as `width: 100%` or not? > > > > Yes the effect in this case will be same, but I think maybe we should use > > flexbox related properties where possible ? > > I'm torn on this one but I'd say we should minimize the use of flexbox to make > it easier to support Safari. ack, done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:852: height: 290px; On 2016/04/22 16:36:23, Thomas Greiner wrote: > On 2016/04/20 14:28:59, saroyanm wrote: > > On 2016/04/18 18:32:52, Thomas Greiner wrote: > > > Detail: Shouldn't this be `240px`? (i.e. 6 * 40px) > > > > Why you think this should be 240px ? I make it so to relatively be of same > > height as the textarea when swithching the views. > > It's based on two pieces of information from the style guide: > - The height of a list element is `40px` (see > https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide...) > - Six elements should be visible in the custom filters list (see > https://issues.adblockplus.org/attachment/ticket/1526/advanced_2_77.png) well spotted! Done. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:853: width: auto; On 2016/04/22 16:36:23, Thomas Greiner wrote: > On 2016/04/20 14:28:59, saroyanm wrote: > > On 2016/04/18 18:32:51, Thomas Greiner wrote: > > > Regression: This will cause longer filters to expand the width of the filter > > > input box and by that also the rest of the page. > > > > We are using overflow: hidden, but while it's the initial value of the item > > removing this doesn't affect anything. > > But not sure if the behavior should be changed ? > > `overflow: hidden` doesn't do anything if you don't specify the width. So what > we should do is specify the width instead of setting it to `auto`. Done, but not really sure if I understood your point in this case, will be thankful if you can show me on the example on your laptop. I made it 100%, but I assume you meant to have an explicit size ? https://codereview.adblockplus.org/29336364/diff/29340648/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29340648/skin/options.css#ne... skin/options.css:316: #content h2 > * On 2016/04/22 16:36:24, Thomas Greiner wrote: > Detail: We should avoid the star selector whenever possible so why not just add > it to the existing `.tooltip` rule? Done.
I added more details about the one remaining regression. The rest looks fine. https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:853: width: auto; On 2016/04/25 08:53:42, saroyanm wrote: > On 2016/04/22 16:36:23, Thomas Greiner wrote: > > On 2016/04/20 14:28:59, saroyanm wrote: > > > On 2016/04/18 18:32:51, Thomas Greiner wrote: > > > > Regression: This will cause longer filters to expand the width of the > filter > > > > input box and by that also the rest of the page. > > > > > > We are using overflow: hidden, but while it's the initial value of the item > > > removing this doesn't affect anything. > > > But not sure if the behavior should be changed ? > > > > `overflow: hidden` doesn't do anything if you don't specify the width. So what > > we should do is specify the width instead of setting it to `auto`. > > Done, but not really sure if I understood your point in this case, will be > thankful if you can show me on the example on your laptop. > I made it 100%, but I assume you meant to have an explicit size ? Hm, setting `width: 100%` doesn't seem to have fixed the issue. Here are the reproduction steps: 1. Go to "Advanced / Edit or create own filter list" 2. Add a long filter (e.g. more than 150 characters) using the text input field below custom filters list 3. Switch to "Overview" tab 4. Switch back to "Edit or create own filter list" tab Observed behavior: Width of custom filters list is larger than before adding the filter. Expected behavior: Width of custom filters list is the same as before adding the filter.
https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29339606/skin/options.css#ne... skin/options.css:853: width: auto; On 2016/04/27 17:16:20, Thomas Greiner wrote: > On 2016/04/25 08:53:42, saroyanm wrote: > > On 2016/04/22 16:36:23, Thomas Greiner wrote: > > > On 2016/04/20 14:28:59, saroyanm wrote: > > > > On 2016/04/18 18:32:51, Thomas Greiner wrote: > > > > > Regression: This will cause longer filters to expand the width of the > > filter > > > > > input box and by that also the rest of the page. > > > > > > > > We are using overflow: hidden, but while it's the initial value of the > item > > > > removing this doesn't affect anything. > > > > But not sure if the behavior should be changed ? > > > > > > `overflow: hidden` doesn't do anything if you don't specify the width. So > what > > > we should do is specify the width instead of setting it to `auto`. > > > > Done, but not really sure if I understood your point in this case, will be > > thankful if you can show me on the example on your laptop. > > I made it 100%, but I assume you meant to have an explicit size ? > > Hm, setting `width: 100%` doesn't seem to have fixed the issue. Here are the > reproduction steps: > > 1. Go to "Advanced / Edit or create own filter list" > 2. Add a long filter (e.g. more than 150 characters) using the text input field > below custom filters list > 3. Switch to "Overview" tab > 4. Switch back to "Edit or create own filter list" tab > > Observed behavior: Width of custom filters list is larger than before adding the > filter. > Expected behavior: Width of custom filters list is the same as before adding the > filter. Now I see the problem, thanks for the reproduction steps, Done.
https://codereview.adblockplus.org/29336364/diff/29340923/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29340923/skin/options.css#ne... skin/options.css:806: width: 838px; This is a workaround. 1. According to https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... the content's inner width should be `840px` (i.e. `900-2*60`). That is excluding borders. 2. The width should be defined by the content element. So I looked into possible solutions and found this to work best: #content { - min-width: 960px; + width: 960px; } It's still debateable whether we should use `960px` or `962px` due to the border width - unlike in the style guide - being included in the width calculation of the box model. Note that I'm not sure why we've been using "min-width" in the first place.
https://codereview.adblockplus.org/29336364/diff/29340923/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29336364/diff/29340923/skin/options.css#ne... skin/options.css:806: width: 838px; On 2016/04/28 10:48:44, Thomas Greiner wrote: > This is a workaround. > > 1. According to > https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide... > the content's inner width should be `840px` (i.e. `900-2*60`). That is excluding > borders. Yes, true, Done. > 2. The width should be defined by the content element. So I looked into possible > solutions and found this to work best: > > #content > { > - min-width: 960px; > + width: 960px; > } Done. > It's still debateable whether we should use `960px` or `962px` due to the border > width - unlike in the style guide - being included in the width calculation of > the box model. Yes you are right it's bit tricky with the design example, I kept it 962px for now, otherwise we should change paddings, or content should be 838. > Note that I'm not sure why we've been using "min-width" in the first place.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
