Issue 5482 - Sidebar and about ABP dialog

skin/new-options.css

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 31 matching lines @@
   /* local("Ø") forces using no local font called Source Sans Pro */
   src: local("Ø"), url(fonts/SourceSansPro-Semibold.woff) format("woff");
   font-weight: 600;
   font-style: normal;
 }
 
 body
 {
   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;
 }
 
 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
 {
   -webkit-box-sizing: border-box;
   -moz-box-sizing: border-box;
   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"]
 {
   width: 18px;
   height: 18px;
   margin-top: 0px;
   -moz-margin-end: 20px;
   -webkit-margin-end: 20px;
   padding: 0px;
   border: none;
   background-color: transparent;
   background-position: -51px 0px;
 }
 
 button[role="checkbox"][aria-checked="true"],
 #dialog-body .table button[role="checkbox"][aria-checked="true"]
 {
   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;
   -webkit-margin-start: -1px;
   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
 {
   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
 {
   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;
 }
 
@@ skipping 679 matching lines @@
 }
 
 #dialog h3,
 #dialog label
 {
   font-size: 14px;
   font-weight: 600;
   margin: 0px;
 }
 
-#dialog input[type="text"],
-#dialog input[type="search"]
+#dialog input[type="text"]
 {
   font-size: 16px;
   margin-top: 10px;
   padding: 5px;
   width: 100%;
 }
 
 #dialog .table
 {
   width: 100%;
@@ skipping 11 matching lines @@
   flex: 1;
   font-size: 16px;
   color: #FFFFFF;
 }
 
 #dialog-body .dialog-content-block
 {
   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;
   margin-bottom: 20px;
   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,
 body:not([data-dialog="language-add"]) #dialog-content-language-add,
 body:not([data-dialog="language-change"]) #dialog-title-language-change,
 body:not([data-dialog="language-change"]) #dialog-content-language-change,
 body:not([data-dialog="predefined"]) #dialog-title-predefined,
 body:not([data-dialog="predefined"]) #dialog-content-predefined,
 body:not([data-dialog]) #dialog
 {
   display: none;
 }