issue 2377 - Finish design of Advanced tab of new options page

skin/options.css

Index: skin/options.css
===================================================================
--- a/skin/options.css
+++ b/skin/options.css
@@ -54,7 +54,7 @@
   font-size: 14px;
 }
 
-h1 
+h1
 {
   font-size: 24px;
   line-height: 1em;
@@ -85,7 +85,9 @@
   display: none;
 }
 
-input[type="text"], input[type="search"]
+input[type="text"],
+input[type="search"],
+textarea
 {
   -webkit-box-sizing: border-box;
   -moz-box-sizing: border-box;
@@ -419,12 +421,12 @@
   display: flex;
   margin-bottom: 0px;
   padding: 0px;
+  border-bottom: 1px solid #A1A1A1;
 }
 
 .tabs.horizontal li
 {
   display: inline-block;
-  border-bottom: 1px solid #A1A1A1;
   padding: 10px 46px;
   color: #3A7BA6;
   text-align: center;
@@ -454,6 +456,22 @@
   min-width: 18px;
 }
 
+.icon-save
+{
+  background-position: 0px -61px;
+  cursor: pointer;
+  height: 18px;
+  width: 18px;

[Thomas Greiner, 2016/02/15 18:09:22]

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.

[saroyanm, 2016/02/24 11:23:57]

Fair enough, done.

+}
+
+.icon-cancel
+{
+  background-position: -18px -61px;
+  cursor: pointer;
+  height: 18px;
+  width: 18px;

[Thomas Greiner, 2016/02/15 18:09:21]

Would be nice if we could remove all those duplicated styles if it's not too
much effort for this code review.

[saroyanm, 2016/02/24 11:23:58]

Sure, hopefully I could organize the code better.

+}
+
 .icon-update
 {
   background-position: -34px -0px;
@@ -485,11 +503,11 @@
 
 .controls
 {
+  position: relative;

[Thomas Greiner, 2016/02/15 18:09:23]

Detail: I know that you merely moved this but I'm wondering why it was added
(see r15) because it seems redundant.

[saroyanm, 2016/02/24 11:23:58]

Yes, neither can't see a reason. Done.

   border-top: 1px solid #CDCDCD;
-  padding-top: 8px;
+  padding-top: 14px;
   -moz-padding-start: 16px;
   -webkit-padding-start: 16px;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: According to the style guide there should be `16px` padding both before
and after the "save" and "cancel" buttons.

[saroyanm, 2016/02/24 11:23:58]

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.

-  position: relative;
 }
 
 .controls > div
@@ -509,11 +527,11 @@
 
 .controls button span:not(.icon)

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: The button text is smaller than `14px`.

[saroyanm, 2016/02/24 11:23:58]

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.

 {
+  display: inline-block;
+  -moz-margin-start: 16px;
+  -webkit-margin-start: 16px;
+  padding-top: 1px;
   color: #3A7BA6;
-  display: inline-block;
-  -moz-margin-start: 15px;
-  -webkit-margin-start: 15px;
-  padding-top: 1px;
   vertical-align: top;
 }
 
@@ -606,6 +624,24 @@
   Advanced tab content
 */
 
+h1.with-description,
+h3.with-description

[Thomas Greiner, 2016/02/15 18:09:21]

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>`

[saroyanm, 2016/02/24 11:23:58]

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.

+{
+  display: flex;
+  align-items: baseline;
+}
+
+#tweaks
+{
+  margin-top: 18px;

[Thomas Greiner, 2016/02/15 18:09:22]

It seems like this margin is the same for each `h1` tag so please apply this
style more generically.

[saroyanm, 2016/02/24 11:23:58]

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.

+}
+
+#tweaks li
+{
+  padding-top: 6px;
+  padding-bottom: 6px;

[Thomas Greiner, 2016/02/15 18:09:22]

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?

[saroyanm, 2016/02/24 11:23:58]

Done.

+}
+
 #filter-lists > div
 {
   display: none;
@@ -643,6 +679,11 @@
   width: 330px;
 }
 
+#own-filter-list span:first-child
+{
+  font-weight: 600;
+}

[Thomas Greiner, 2016/02/15 18:09:22]

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.

[saroyanm, 2016/02/24 11:23:58]

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.

+
 .table-header
 {
   display: flex;
@@ -650,9 +691,8 @@
     
 #filter-lists h3

[Thomas Greiner, 2016/02/15 18:09:22]

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.

[saroyanm, 2016/02/24 11:23:57]

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.

 {
-  display: inline-block;
   margin-bottom: 7px;
-  font-size: 14px;
+  font-size: 16px;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: Why not make this apply globally for all `<h3>` tags?

[saroyanm, 2016/02/24 11:23:58]

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.

 }
 
 .table-header h3:first-child
@@ -740,13 +780,14 @@
 
 #custom-filters-header
 {
-  padding: 0px 20px;
+  padding: 0px 16px;
   margin-bottom: 10px;
 }
 
 #custom-filters-raw-controls
 {
   display: flex;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: This seems redundant because `.controls > div` already defines it.

[saroyanm, 2016/02/24 11:23:59]

Done.

+  justify-content: space-between;
 }
 
 #custom-filters:not(.mode-edit) #custom-filters-raw,
@@ -757,11 +798,6 @@
   display: none;
 }
 
-#custom-filters-raw-save
-{
-  padding: 0px 16px;
-}
-
 #custom-filters .table
 {
   height: 290px;
@@ -785,6 +821,8 @@
 {
   width: 100%;
   height: 100%;
+  -moz-padding-start: 16px;
+  -webkit-padding-start: 16px;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: Any reason why you're only setting the padding on one side but not the
other?

Same question for the change below.

[saroyanm, 2016/02/24 11:23:58]

Done.

 }
 
 #custom-filters-wrapper
@@ -799,8 +837,8 @@
   border-top-style: solid;
   border-color: #1E8728;
   height: 25px;
-  -moz-padding-start: 10px;
-  -webkit-padding-start: 10px;
+  -moz-padding-start: 16px;
+  -webkit-padding-start: 16px;
   -moz-padding-end: 60px;
   -webkit-padding-end: 60px;
   padding-top: 18px;
@@ -874,15 +912,16 @@
 
 .tooltip, #block-element-explanation a
 {
+  position: relative;
+  margin: 0px 4px;
   border-bottom: dashed 1px;
+  height: 15px;
   color: #3A7BA6;
+  font-size: 12px;
+  line-height: 19px;
+  text-decoration: none;
+  font-weight: 400;
   cursor: default;
-  font-size: 12px;
-  height: 15px;
-  line-height: 19px;
-  margin: 0px 4px;
-  position: relative;
-  text-decoration: none;
 }
 
 .context-menu