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,22 +49,21 @@
 {
   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;
   color: #494949;
 }
 
 h1
 {
-  font-size: 24px;
-  line-height: 1em;
+  font-size: 48px;

[saroyanm, 2017/08/10 12:04:48]

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

[juliandoucette, 2017/08/16 23:42:31]

Acknowledged.

Thank you for pointing this out.

[saroyanm, 2017/08/18 12:44:28]

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.

[juliandoucette, 2017/08/21 13:06:39]

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.

   font-weight: 300;
 }
 
 h2
 {
-  font-size: 16px;
+  font-size: 22px;
   font-weight: 600;
 }
 
@@ -73,26 +72,12 @@
   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
 {
   -webkit-box-sizing: border-box;
@@ -100,6 +85,65 @@
   box-sizing: border-box;
 }
 
+/*
+  Buttons
+ */
+
+button.primary,

[juliandoucette, 2017/08/16 23:42:32]

NIT: I prefer adding .button to <button>s and styling *just* .button here and
.button.primary below.

[saroyanm, 2017/08/18 10:23:13]

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

[juliandoucette, 2017/08/18 11:17:04]

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.

[saroyanm, 2017/08/18 12:44:28]

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

[juliandoucette, 2017/08/21 13:06:39]

Acknowledged.

FWIW: I was originally proposing to have button.button. And I added another
different suggestion after you rejected my button.button NIT suggestion.

+button.secondary,
+.button.primary,
+.button.secondary
+{
+  padding: 0.8em 1.2em;
+  font-size: 18px;
+  font-weight: 600;
+  text-decoration: none;
+  text-transform: uppercase;
+}
+
+button.primary,
+.button.primary
+{
+  border: none;
+  color: #FFF;
+  background-color: #0A9DD1;
+  cursor: pointer;
+}
+
+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;
+}
+
+#about,
+a

[juliandoucette, 2017/08/16 23:42:32]

Did you mean to style all <a> here?

(It doesn't look like it)

[saroyanm, 2017/08/18 10:23:13]

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.

[saroyanm, 2017/08/18 12:44:28]

Done.

+{
+  text-decoration: none;
+  color: #099CD0;
+  cursor: pointer;
+  font-size: 20px;
+  text-align: center;
+}
+
 button[role="checkbox"],
 #dialog-body .table button[role="checkbox"]
 {
@@ -120,63 +164,83 @@
   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: 0;
   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

[juliandoucette, 2017/08/16 23:42:32]

NIT: You could have just margin: 0 2em; above

[saroyanm, 2017/08/18 10:23:13]

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

[juliandoucette, 2017/08/18 11:17:04]

Acknowledged.

 {
-  margin: 0px;
-  font-size: 16px;
-  line-height: 1em;
+  margin-left: 2em;
 }
 
-#page-title h1
+#sidebar header h1
 {
-  margin: 0px;
-  padding: 8px 0px 16px 0px;
+  font-size: 24px;
+  margin: 0;
 }
 
-.tabs li
+#sidebar header p
 {
-  cursor: pointer;
-  display: flex;
+  font-size: 44px;
+  margin: 0;

[juliandoucette, 2017/08/16 23:42:32]

NIT: I thought we'd agreed against unitless values?

[saroyanm, 2017/08/18 10:23:13]

Noted, will fix.

[saroyanm, 2017/08/18 12:44:28]

Done.

 }
 
-.tabs li a
+html[dir="rtl"] #sidebar header
 {
-  flex: 1;
-  color: inherit;
-  text-decoration: none;
+  text-align: left;
 }
 
-.tabs.vertical
+#sidebar-logo

[juliandoucette, 2017/08/16 23:42:32]

NIT: It's kindof inconsistent that you are styling this image so specifically
and those nav and footer so generally.

[saroyanm, 2017/08/18 10:23:13]

I agree, I'll change.

[saroyanm, 2017/08/18 12:44:28]

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.

[juliandoucette, 2017/08/21 13:06:39]

Acknowledged.

I agree. Good thinking.

+{
+  width: 3em;
+  margin-bottom: 0.9em;
+}
+
+#sidebar nav,
+#sidebar footer
+{
+  margin: 1.4em 0;
+}
+
+[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,102 +248,58 @@
   border-color: #CDCDCD transparent;
   border-style: solid;
   border-width: 1px;
-  font-size: 16px;
   font-weight: 300;
-  line-height: 1em;
+  cursor: pointer;

[juliandoucette, 2017/08/16 23:42:31]

Is there a reason that you are using <spans> inside of <li> here instead of <a>?

[saroyanm, 2017/08/18 10:23:13]

No this is styles updated on top of old implementation of tabs, I agree that
having <a> instead of <span>s is correct here.

[saroyanm, 2017/08/18 12:44:28]

Done.

 }
 
-.tabs.vertical li a,
-.tabs.vertical li span,
-.tabs.vertical li::after
+[role="tablist"] li a
 {
-  margin: auto 20px;
+  flex: 1;
+  color: inherit;
+  text-decoration: none;

[juliandoucette, 2017/08/16 23:42:32]

NIT/Note: You could default to none and add decoration to `#content a` or
otherwise.

[saroyanm, 2017/08/18 10:23:13]

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

[saroyanm, 2017/08/18 12:44:29]

Done.

 }
 
-.tabs li[role="tab"]:focus
+[role="tablist"] li a,
+[role="tablist"] li span,
+[role="tablist"] li::after
+{
+  margin: auto 0.8em;

[juliandoucette, 2017/08/16 23:42:32]

NIT: I don't think auto does anything here. If I am correct than it may be
misleading. 

[saroyanm, 2017/08/18 10:23:13]

It's not, well spotted.

[saroyanm, 2017/08/18 12:44:28]

Done.

+}
+
+[role="tablist"] li[role="tab"]:focus
 {
   outline: none;
   text-shadow: 0px 0px 1px #888;

[juliandoucette, 2017/08/16 23:42:32]

NIT/Note: This doesn't look so hot on [lowdpi, chromium, debain]

[saroyanm, 2017/08/18 10:23:13]

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.

[juliandoucette, 2017/08/18 11:17:04]

Acknowledged.

 }
 
-.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
+footer

[juliandoucette, 2017/08/16 23:42:32]

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

[saroyanm, 2017/08/18 10:23:13]

Currently we only have 1 footer, so I did it general. 
but I'll make this more specific.

[saroyanm, 2017/08/18 12:44:29]

Done.

 {
   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
+footer p
 {
-  content: "";
-  min-width: 14px;
-  height: 14px;
-  background-image: url(options-sprite.png);
+  display: flex;
+  justify-content: center;
+  margin: 1.2em 0;
 }
 
-#tab-general::after
+#about
 {
-  background-position: -16px -18px;
-}
-
-#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]
-{
-  display: none;
-}
-
-#tab-share:lang(zh) + li,
-#tab-share[hidden] + li
-{
-  border-top: none;
+  border:none;
+  background-color: transparent;
+  font-weight: 300;
+  font-family: inherit;
 }
 
 #content
@@ -287,8 +307,8 @@
   background-color: #FFFFFF;
   border: 1px solid #CDCDCD;
   border-radius: 8px;
-  width: 840px;
-  padding: 0px 60px 40px 60px;
+  max-width: 46.3em;

[juliandoucette, 2017/08/16 23:42:32]

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.

[saroyanm, 2017/08/18 10:23:12]

Yes, you are right this should be "border-box"

[saroyanm, 2017/08/18 12:44:28]

Done.

+  padding: 0px 2em 1.4em 2em;
 }
 
 #content h1
@@ -298,20 +318,6 @@
   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;
@@ -1011,8 +1017,7 @@
   margin: 0px;
 }
 
-#dialog input[type="text"],
-#dialog input[type="search"]
+#dialog input[type="text"]
 {
   font-size: 16px;
   margin-top: 10px;
@@ -1044,20 +1049,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 +1056,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,