Issue 5482 - Sidebar and about ABP dialog

skin/new-options.css

Index: skin/new-options.css
===================================================================
--- a/skin/new-options.css
+++ b/skin/new-options.css
@@ -49,57 +49,105 @@
 {
   background-color: #F5F5F5;
   display: flex;
-  margin: 20px 10px;
+  margin: 1.2em 0.3em;
   font-family: "Source Sans Pro", sans-serif;
-  font-size: 14px;
+  font-size: 20px;
+  font-weight: 300;

[juliandoucette, 2017/08/21 14:10:35]

This thin body font requires aliasing and is hard to read at this size;
especially at lowDPI. 

[saroyanm, 2017/08/21 15:20:22]

As mentioned, on other file, styleguide enhancements, suggestions should be part
of bitbucket repository.

This review reflects the styleguide on top of old implementation.

[juliandoucette, 2017/08/21 16:07:25]

I can't find "300" or "thin" in the styleguide or spec anywhere?

It just says "Regular" (which is normally 400)?

[saroyanm, 2017/08/21 16:46:13]

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.

[juliandoucette, 2017/08/22 10:10:44]

Acknowledged.

I think it's worth bringing up.

   color: #494949;
 }
 
 h1
 {
-  font-size: 24px;
-  line-height: 1em;
+  font-size: 3em;
   font-weight: 300;
 }
 
 h2
 {
-  font-size: 16px;
+  font-size: 1.375em;
   font-weight: 600;
 }
 
-p
-{
-  font-weight: 300;
-}
-
-hr
-{
-  background-color: #CDCDCD;
-  border: 0px;
-  height: 1px;
-  margin: 0px;
-}
-
 [aria-hidden="true"]
 {
   display: none !important;
 }
 
-input[type="search"]::-webkit-search-cancel-button
-{
-  display: none;
-}
-
 input[type="text"],
-input[type="search"],
-textarea
+textarea,
+main
 {
   -webkit-box-sizing: border-box;
   -moz-box-sizing: border-box;
   box-sizing: border-box;
 }
 
+/*
+  Buttons and links
+ */
+
+a,
+button.link
+{
+  text-decoration: none;
+  color: #099CD0;
+  font-size: 1.25em;

[juliandoucette, 2017/08/21 14:10:34]

It seem strange to make anchors slightly larger than body text :/

[saroyanm, 2017/08/21 15:20:23]

I agree, this is not what styleguide says.

[juliandoucette, 2017/08/21 16:07:25]

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.

[saroyanm, 2017/08/21 16:46:13]

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.

[juliandoucette, 2017/08/22 10:10:44]

Acknowledged.

[saroyanm, 2017/08/23 13:35:45]

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

+  cursor: pointer;
+}
+
+button.link
+{
+  border:none;

[juliandoucette, 2017/08/21 14:10:34]

NIT: Missing space, + I thought we agreed on border: 0?

[saroyanm, 2017/08/21 15:20:22]

Good point.

[saroyanm, 2017/08/23 13:35:45]

Done.

+  background-color: transparent;
+  font-weight: 300;
+  font-family: inherit;
+}
+
+button.primary,

[juliandoucette, 2017/08/21 14:10:34]

These seem like styles that should apply to button, .button without .primary and
.secondary?

[saroyanm, 2017/08/21 15:20:23]

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.

[juliandoucette, 2017/08/21 16:07:25]

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.

[saroyanm, 2017/08/21 16:46:13]

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.

[juliandoucette, 2017/08/22 10:10:44]

Acknowledged.

[saroyanm, 2017/08/23 13:35:46]

Done.

+button.secondary,
+.button.primary,
+.button.secondary
+{
+  padding: 0.8em 1.2em;
+  font-size: 1.125em;
+  font-weight: 600;
+  text-decoration: none;
+  text-transform: uppercase;
+}
+
+button.primary,
+.button.primary
+{
+  border: none;
+  color: #FFF;
+  background-color: #0A9DD1;
+  cursor: pointer;

[juliandoucette, 2017/08/21 14:10:34]

I think this is redundant?

[saroyanm, 2017/08/21 15:20:22]

Why ?

[juliandoucette, 2017/08/21 16:07:25]

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.)

[saroyanm, 2017/08/21 16:46:13]

Yes, I think it should.

[saroyanm, 2017/08/23 13:35:45]

Done.

+}
+
+button.primary:hover,
+.button.primary:hover
+{
+  box-shadow: inset 0 0 0 2px #005D80;
+}
+
+button.primary[disabled]
+{
+  background-color: #5CBCE1;
+}
+
+button.secondary,
+.button.secondary
+{
+  border: 1px solid #0A9DD1;
+  color: #099CD0;
+}
+
+button.secondary:hover,
+.button.secondary:hover
+{
+  box-shadow: inset 0 0 0 1px #099CD0;
+}
+
 button[role="checkbox"],
 #dialog-body .table button[role="checkbox"]
 {
@@ -120,63 +168,84 @@
   background-position: -68px 0px;
 }
 
-#nav-sidebar
+/*
+  Sidebar
+ */
+
+#sidebar,
+#sidebar .fixed,
+[role="tablist"]
 {
-  min-width: 198px;
+  width: 13.2em;
 }
 
-#nav-sidebar .fixed
+#sidebar
 {
-  top: 40px;
-  bottom: 2px;
+  flex-shrink: 0; 
+}
+
+#sidebar .fixed
+{
+  top: 1.2em;
+  bottom: 0em;
   height: auto;
   position: fixed;
 }
 
-#page-title
+#sidebar header
 {
-  padding: 0px 20px;
+  text-align: right;
+  margin-right: 2em;
 }
 
-#page-title p
+html[dir="rtl"] #sidebar header
 {
-  margin: 0px;
-  font-size: 16px;
-  line-height: 1em;
+  margin-left: 2em;
 }
 
-#page-title h1
+#sidebar header h1
 {
-  margin: 0px;
-  padding: 8px 0px 16px 0px;
+  margin: 0em;
+  font-size: 1.2em;
 }
 
-.tabs li
+#sidebar header h2
 {
-  cursor: pointer;
-  display: flex;
+  margin: 0em;
+  font-size: 2.2em;
+  font-weight: 300;
 }
 
-.tabs li a
+html[dir="rtl"] #sidebar header
 {
-  flex: 1;
-  color: inherit;
-  text-decoration: none;
+  text-align: left;
 }
 
-.tabs.vertical
+#sidebar-logo
+{
+  width: 3em;
+  margin-bottom: 0.9em;
+}
+
+#sidebar nav,
+#sidebar footer
+{
+  margin: 1.4em 0em;
+}
+
+[role="tablist"]
 {
   list-style: none;
   margin: 0px;
   position: relative;
   padding: 0px;
-  width: 198px;
 }
 
-.tabs.vertical li
+[role="tablist"] li
 {
-  height: 46px;
+  display: flex;
   margin-top: -1px;
+  padding: 1em 0px;
   -moz-margin-end: -1px;
   -webkit-margin-end: -1px;
   -moz-margin-start: -1px;
@@ -184,144 +253,73 @@
   border-color: #CDCDCD transparent;
   border-style: solid;
   border-width: 1px;
-  font-size: 16px;
   font-weight: 300;
-  line-height: 1em;
+  cursor: pointer;
 }
 
-.tabs.vertical li a,
-.tabs.vertical li span,
-.tabs.vertical li::after
+[role="tablist"] li a
 {
-  margin: auto 20px;
+  flex: 1;
+  color: inherit;
 }
 
-.tabs li[role="tab"]:focus
+[role="tablist"] li a,
+[role="tablist"] li span,
+[role="tablist"] li::after
+{
+  margin: 0em 0.8em;
+}
+
+[role="tablist"] li[role="tab"]:focus,
+[role="tablist"] li[role="tab"] a:focus
 {
   outline: none;
   text-shadow: 0px 0px 1px #888;
 }
 
-.tabs li[role="tab"][aria-selected]
+li[role="tab"][aria-selected]
 {
-  border-radius: 3px 0px 0px 3px;
   -moz-border-start-color: #CDCDCD;
   -webkit-border-start-color: #CDCDCD;
   font-weight: 600;
   background-color: #FFF;
 }
 
-html[dir="rtl"] .tabs li[role="tab"][aria-selected]
-{
-  border-radius: 0px 3px 3px 0px;
-}
-
-.tabs.vertical.bottom
+#sidebar footer
 {
   bottom: 0px;
   position: absolute;
-}
-
-.tabs.vertical.bottom li:first-child
-{
-  border-top: 0px;
-}
-
-#tab-contribute
-{
-  border-bottom: none;
-}
-
-#nav-sidebar ul li span
-{
   width: 100%;
 }
 
-#nav-sidebar ul li::after
+#sidebar footer p
 {
-  content: "";
-  min-width: 14px;
-  height: 14px;
-  background-image: url(options-sprite.png);
+  display: flex;
+  justify-content: center;
+  margin: 1.2em 0em;
 }
 
-#tab-general::after
+main
 {
-  background-position: -16px -18px;
+  background-color: #FFFFFF;
+  border: 1px solid #CDCDCD;
+  border-radius: 8px;
+  max-width: 46.3em;
+  padding: 0px 2em 1.4em 2em;
 }
 
-#tab-advanced::after
-{
-  background-position: -46px -18px;
-}
-
-#tab-help::after
-{
-  background-position: -1px -18px;
-}
-
-#tab-share::after
-{
-  background-position: -61px -18px;
-}
-
-#tab-contribute::after
-{
-  background-position: -31px -18px;
-}
-
-#tab-share:lang(zh),
-#tab-share[hidden]
+main > div
 {
   display: none;
 }
 
-#tab-share:lang(zh) + li,
-#tab-share[hidden] + li
-{
-  border-top: none;
-}
-
-#content
-{
-  background-color: #FFFFFF;
-  border: 1px solid #CDCDCD;
-  border-radius: 8px;
-  width: 840px;
-  padding: 0px 60px 40px 60px;
-}
-
-#content h1
+main h1
 {
   border-bottom: 1px solid #CDCDCD;
   margin: 0px 0px 24px 0px;
   padding: 40px 0px 16px 0px;
 }
 
-#link-version
-{
-  display: flex;
-  margin: 12px 20px;
-  color: #3A7BA6;
-  text-decoration: none;
-}
-
-#abp-version
-{
-  -moz-margin-start: 5px;
-  -webkit-margin-start: 5px;
-}
-
-#content-wrapper
-{
-  position: relative;
-}
-
-#content-wrapper > div
-{
-  display: none;
-}
-
 body[data-tab|="general"] #content-general,
 body[data-tab|="advanced"] #content-advanced,
 body[data-tab|="whitelist"] #content-whitelist,
@@ -1011,8 +1009,7 @@
   margin: 0px;
 }
 
-#dialog input[type="text"],
-#dialog input[type="search"]
+#dialog input[type="text"]
 {
   font-size: 16px;
   margin-top: 10px;
@@ -1044,20 +1041,6 @@
   padding: 12px 0px;
 }
 
-#dialog-body button
-{
-  background-color: #F5F5F5;
-  border: none;
-  color: #3A7BA6;
-  cursor: pointer;
-  display: block;
-  font-family: inherit;
-  margin-top: 8px;
-  padding: 10px 16px;
-  text-align: initial;
-  width: 100%;
-}
-
 #dialog .url
 {
   margin-top: 10px;
@@ -1065,6 +1048,8 @@
   word-wrap: break-word;
 }
 
+body:not([data-dialog="about"]) #dialog-title-about,
+body:not([data-dialog="about"]) #dialog-content-about,
 body:not([data-dialog="custom"]) #dialog-title-custom,
 body:not([data-dialog="custom"]) #dialog-content-custom,
 body:not([data-dialog="language-add"]) #dialog-title-language-add,