No Issue - Implemented first run page

skin/firstRun.css

 /*

[Thomas Greiner, 2018/02/16 16:19:15]

It is difficult to distinguish styles that are coming from websites-defaults
from styles you introduced for the purpose of the first-run page. However, both
are mixed together in "firstRun.css", implying that all of those were made
specifically for the first-run page.

Therefore, I strongly suggest to split those styles up and only include styles
specifically created for the first-run page in here.

Thereby you can keep any non-relevant styles (e.g. ones targeting IE9) but in a
separate file so that we know that we've simply imported them from Websites and
don't need to change them.

- * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-present 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
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-@font-face {
-  font-family: 'Source Sans Pro';
-  font-style: normal;
-  font-weight: 300;
-  font-stretch: normal;
-  src: local ("Ø"),

[juliandoucette, 2017/10/08 13:18:12]

NIT: What the?

[martin, 2017/10/16 09:09:09]

My reaction exactly. It was like this in the original source, so I decided to
keep it that way. Let me know what it does.

[saroyanm, 2017/10/16 10:22:59]

It's used to force not using local font, I think the previous comments applies
here as well.
https://www.paulirish.com/2009/bulletproof-font-face-implementation-syntax/#s...

-  url(fonts/SourceSansPro-Light.woff) format("otf");
-}
-
-@font-face {
-  font-family: 'Source Sans Pro';
+  * This file is part of Adblock Plus <https://adblockplus.org/>,
+  * Copyright (C) 2006-present 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
+  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+  * GNU General Public License for more details.
+  *
+  * You should have received a copy of the GNU General Public License
+  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+*/

[Thomas Greiner, 2018/02/16 16:19:15]

Detail: What happened to the whitespaces here?

+
+@font-face 
+{
+  font-family: "Source Sans Pro";
   font-style: normal;
   font-weight: 400;
   font-stretch: normal;
   src: local ("Ø"),
   url(fonts/SourceSansPro-Regular.woff) format("woff");
 }
 
-@font-face {
-  font-family: 'Source Sans Pro';
-  font-style: normal;
+@font-face
+{
+  font-family: "Source Sans Pro";
+  font-style: bold;
   font-weight: 700;
   font-stretch: normal;
   src: local ("Ø"),
   url(fonts/SourceSansPro-bold.woff) format("woff");
 }
 
-
-body {
-  font-family: 'Source Sans Pro', Helvetica, Arial, sans-serif;
-  font-size: 16px;

[juliandoucette, 2017/10/08 13:18:11]

NIT: I prefer to set document wide styles on the root (html) element.
(Acknowledging that it doesn't make a difference to this CSS e.g. because you
are not using rem.)

[martin, 2017/10/16 14:36:00]

Done.

-  background-color: white;

[juliandoucette, 2017/10/08 13:18:11]

NIT/Reminder: "CSS color values should be specified in hexadecimal where
possible." (also, "#fff" is fewwer characters than "white" :P)

[martin, 2017/10/16 09:09:09]

Done.

+html
+{
+  font-family: "Source Sans Pro", Helvetica, Arial, sans-serif;
+}
+
+body
+{
   margin: 0;
   padding: 0;
 }
 
-.container {
-  width: 100%;
-  max-width: 100%;
-  margin: 0px auto;
-  padding: 0px 1em; 
-}
-
-header {

[juliandoucette, 2017/10/08 13:18:11]

NOTE: Padding inside the .content body causes this to be offcenter ~400px

+.ta-centre
+{
+  text-align: center;
+}
+
+#content {
+  margin-bottom: 3em;
+}
+
+@media (max-width: 992px) 
+{
+  #content .one-third 
+  {
+    padding: 3em 2em;
+    border-bottom: 1px solid #d3d3d3;
+  }
+  #content .one-third:last-child {
+    border-bottom: none;
+  }
+}
+
+/******************************************************************************
+  * .content
+*****************************************************************************/

[Thomas Greiner, 2018/02/16 16:19:17]

Detail: Missing whitespace at beginning of line causing the line to be offset a
bit.

Also applies to other occurrences in this file.

+
+.content h1,
+.content h2,
+.content h3,
+.content h4,
+.content h5
+{
+  font-weight: bold;

[Thomas Greiner, 2018/02/16 16:19:19]

We consistently use number values for the font weight across all our files to
avoid confusion so please use `700` here.

+  margin: 32px 0px;

[Thomas Greiner, 2018/02/16 16:19:19]

Coding style: "Omit unit specification after “0” values, unless required."

Also applies to other occurrences in this file.

See https://google.github.io/styleguide/htmlcssguide.html#0_and_Units

+  margin: 2rem 0rem;
+}
+
+.content h1 { font-size: 1.8em; }
+.content h2 { font-size: 1.4em; }
+.content h3 { font-size: 1.3em; }
+.content h4 { font-size: 1.2em; }
+.content h5 { font-size: 1.1em; }
+
+@media(min-width: 768px)
+{
+  .content h1 { font-size: 2.4em; }
+  .content h2 { font-size: 1.6em; }
+}
+
+.content a,
+.content a:visited
+{
+  color: #C70D2C;
+}
+
+/*******************************************************************************
+ * #navbar
+ ******************************************************************************/
+
+#navbar
+{
+  min-height: 4em;
+  background-color: #c70d2c;
+}
+
+@media(max-width: 991px)
+{
+  #navbar
+  {
+    overflow: auto;
+  }
+}
+
+#navbar li
+{
+  list-style-type: none;
+}
+
+#navbar .container
+{
+  padding-left: 0;
+  padding-right: 0;
+}
+
+/* #navbar #navbar-logo
+ ******************************************************************************/
+
+#navbar-logo
+{
+  /* padding is visible in hover background-color change */
+  padding: 0em 1em;
+  color: #fff;
+}
+
+#navbar-logo:hover,
+#navbar-logo:active,
+#navbar-logo:focus
+{
+  background-color: #AE0013;
+  text-decoration: none;
+}
+
+#navbar-logo,
+#navbar-logo > *
+{
+  float: left;
+}
+
+[dir="rtl"] #navbar-logo,
+[dir="rtl"] #navbar-logo > *
+{
+  float: right;
+}
+
+#navbar-logo img
+{
+  height: 4em;
+  padding: 0.5em 0em;
+  margin: 0em 1em 0em 0em;
+}
+
+[dir="rtl"] #navbar-logo > img
+{
+  margin: 0em  0em 0em 1em;
+}
+
+#navbar-logo > span
+{
+  /* full-height: 2.91 * 1.375 = ~4em */
+  line-height: 2.91em;
+  font-size: 1.375em;
+}
+
+/* #navbar #navbar-menu-toggle
+ ******************************************************************************/
+
+#navbar-menu-toggle
+{
+  float: right;
+  padding: 1em;
+}
+
+[dir="rtl"] #navbar-menu-toggle
+{
+  float: left;
+}
+
+/* show on mobile */
+#navbar-menu-toggle,
+#navbar-menu-toggle > img
+{
+  display: block;
+}
+
+/* hide on desktop */
+@media(min-width: 992px)
+{
+  #navbar-menu-toggle
+  {
+    display: none;
+  }
+}
+
+#navbar-menu-toggle > img
+{
+  /* plus 2em padding = 4em */
+  height: 2em;
+}
+
+#navbar-menu-toggle:hover,
+#navbar-menu-toggle:active,
+#navbar-menu-toggle:focus
+{
+  background-color: #AE0013;
+}
+
+/* #navbar #navbar-menu
+ ******************************************************************************/
+
+#navbar-menu a
+{
+  display: block;
+  color: #fff;
+  text-decoration: none;
+}
+
+/* #navbar #navbar-menu #navbar-locale-menu
+ ******************************************************************************/
+
+/* caret */
+#navbar-locale-selected:after
+{
+  display: inline-block;
+  width: 0;
+  height: 0;
+  margin: 0em 0em 0em 0.255em;
+  vertical-align: .255em;

[Thomas Greiner, 2018/02/16 16:19:16]

Coding style: "Don't omit the optional leading 0 for decimal numbers."

Also applies to other occurrences in this file.

+  content: "";
+  border-top: .3em solid;
+  border-right: .3em solid transparent;
+  border-left: .3em solid transparent;
+}
+
+[dir="rtl"] #navbar-locale-selected:after
+{
+  margin: 0em 0.255em 0em 0em;
+}
+
+#navbar-locale-menu
+{
+  display: none;
+}
+
+#navbar-locale-menu.visible
+{
+  display: block;
+}
+
+#navbar-locale-menu > li > a
+{
+  padding: 0.75em 1em;
+}
+
+#navbar-locale-menu > li > a:hover,
+#navbar-locale-menu > li > a:active,
+#navbar-locale-menu > li > a:focus
+{
+  background-color: #434343;
+}
+
+/* #navbar #navbar-menu (desktop)
+ ******************************************************************************/
+
+@media(min-width: 992px)
+{
+  #navbar-menu
+  {
+    float: right;
+  }
+
+  [dir="rtl"] #navbar-menu
+  {
+    float: left;
+  }
+
+  #navbar-menu > li,
+  #navbar-menu > li > a
+  {
+    display: inline-block;
+  }
+
+  #navbar-menu > li > a
+  {
+    padding: 0em 1em;
+    line-height: 4em;
+  }
+
+  #navbar-menu > li > a:hover,
+  #navbar-menu > li > a:active,
+  #navbar-menu > li > a:focus
+  {
+    background-color: #AE0013;
+  }
+
+  /* #navbar #navbar-menu #navbar-locale-menu

[Thomas Greiner, 2018/02/16 16:19:20]

You don't include this section of the navbar so let's not include its styles
either.

+   ****************************************************************************/
+
+  #navbar-locale-menubar
+  {
+    position: relative;
+  }
+
+  #navbar-locale-menu
+  {
+    position: absolute;
+    min-width: 16em;
+    max-height: 20em;
+    max-height: 50vh;
+    top: 100%;
+    right: 0px;
+    overflow: auto;
+    z-index: 9001;
+    background-color: #292929;
+  }
+
+  [dir="rtl"] #navbar-locale-menu
+  {
+    right: auto;
+    left: 0px;
+  }
+}
+
+/* #navbar #navbar-menu (mobile)
+ ******************************************************************************/
+
+@media(max-width: 991px)
+{
+  #navbar-menu
+  {
+    display: none;
+    float: left;

[Thomas Greiner, 2018/02/16 16:19:18]

I don't see a rule for reversing this for right-to-left locales.

+    clear: both;
+    width: 100%;
+    background-color: #292929;
+  }
+
+  #navbar-menu.visible
+  {
+    display: block;
+  }
+
+  #navbar-menu li a
+  {
+    padding: 0.75em 1em;
+  }
+
+  #navbar-menu li a:hover,
+  #navbar-menu li a:active,
+  #navbar-menu li a:focus
+  {
+    background-color: #434343;
+  }
+}
+
+/******************************************************************************
+  * Main Page Content
+*****************************************************************************/
+
+#installation-status-header
+{
   text-align: center;
   background: #f9f9f9;

[Thomas Greiner, 2018/02/16 16:19:20]

Detail: Do you actually want to override other background-* properties or do you
merely want to change the color? Because for the latter, using
"background-color" would make this more maintainable.

Also applies to other occurrences in this file.

   padding: 4em 0;
   margin-bottom: 4em;
 }
 
-@media (min-width: 768px) {
-  .container {
-    width: 720px; 
-  } 
-}
-
-@media (min-width: 992px) {

[juliandoucette, 2017/10/08 13:18:11]

NIT: This doesn't make sense.

- You are setting a width larger than the screen at 992px
    - Which doesn't take effect until > 1194px because the media query below is
more specific
        - Where did you get 1194 from anyway? 1170 + 2em (at 16px) is 1202px?
- This causes the layout to be offcenter at 1200px width

-  .container {
-    width: 1200px;
-  } 
-}
-
-@media (max-width: 1194px) {
-  .container {
-    width: 80%;
-    text-align: center;
-  }
-}
-
-@media (max-width: 474px) {
-  .container {
-    width: 100%;

[juliandoucette, 2017/10/08 13:18:11]

NIT: This kills the padding left/right because html is not box-sizing:
border-box.

-  }
-}
-
-.button-primary {

[juliandoucette, 2017/10/08 13:18:11]

NIT/Suggest: Make this button full-width on small mobile devices

+#installation-status-header > h1 {
+  text-transform: uppercase;
+}
+
+.section-header > h2
+{
+  margin: 1em 0 0;
+}
+
+.button-primary
+{
   display: inline-block;
   text-decoration: none;
   text-transform: uppercase;
-  color: white !important;

[juliandoucette, 2017/10/08 13:18:11]

NIT: This important is unnecessary? 

+  color: #fff !important;

[Thomas Greiner, 2018/02/16 16:19:17]

What's the reason for using `!important` here?

   text-align: center;
   width: 100%;
   border-radius: 6px;
+  padding: 1em 0;
+  background: #C70D2C;
+}
+
+.store-buttons
+{
+  width:  100%;
+  display: flex;
+  align-items: center;
+  justify-content: center;
+}
+
+.store-button
+{
+  display: block;
+  width: 180px;
+  height: 58px;
+  border-radius: 6px;
+  background: #000;
+}
+
+.store-button > img 
+{
   height: 54px;

[juliandoucette, 2017/10/08 13:18:12]

NIT: px [height, width, line-height] kinda defeats the purpose of using em
font-size (acknowledging that this is a very edge cases). You can use em here
(and optionally use margin and padding to vertically centre).

[martin, 2017/10/16 14:36:00]

Done.

-  line-height: 54px;
-  background: #C70D2C;
-}
-
-@media (max-width: 1194px) {
-  .button-primary {
-    width: 50%;

[juliandoucette, 2017/10/08 13:18:11]

NIT: This gets pretty awkward around 336px.

-  }
-}
-
-.store-buttons {
-  height: 54px;
-  width: 100%;
-}
-
-.store-button {
-  display: block;
-  height: 54px;
-  width: 180px;
-}
-
-.applestore-button {
-  background: url("./icons/first-run/appstore-bg.svg");
-  float: left;
+}
+
+.applestore-button
+{
   margin-right: 1em;
 }
 
-.googleplay-button {
-  background: url("./icons/first-run/googleplay-bg.svg");
-  float: left;
-}
-
-@media (max-width: 1194px) {
-  .store-buttons {
-    text-align: center;
-  }
-  .store-button {
-    display: inline-block;

[juliandoucette, 2017/10/08 13:18:11]

NIT: These buttons are not horizontally aligned on small screens (e.g. 320px)
because of inline-block + whitespace.

-    float: none;
-  }
-}
-
-@media (max-width: 1207px) {
-  .applestore-button {
-    margin-right: .5em;
-  }
-}
-
-h1 {

[juliandoucette, 2017/10/08 13:18:11]

NIT: This doesn't fit on the screen ~320 Firefox

+[dir="rtl"] .appstore-button
+{
+  margin-right: 0;
+  margin-left: 1em;
+}
+
+@media (max-width: 1280px) 
+{
+  .store-buttons 
+  {
+    flex-direction: column;
+  }
+  
+  .store-button 
+  {
+    width: 100%;
+  }
+  .applestore-button

[Thomas Greiner, 2018/02/16 16:19:16]

Coding style: "Separate rules by new lines."

See https://google.github.io/styleguide/htmlcssguide.html#Rule_Separation

+  {
+    margin: 0 0 1em;
+  }
+}
+
+#installation-status-header > h1
+{
   font-size: 3em;
   margin: 0;
 }
 
-h2 {
-  font-size: 2em;
-}
-
-p {
-  margin: 0 0 1em 0;
-  font-size: 1.25em;

[juliandoucette, 2017/10/08 13:18:12]

NIT: I see that you followed Paul's stytleguide ~exactly... I would prefer that
all flow content inherited font-size from html (except for heading content). 

-  font-weight: 300;

[juliandoucette, 2017/10/08 13:18:11]

NIT: We agreed (privately - sorry) to use 400 instead of 300 for websites.

-  line-height: 150%;
-}
-
-p > a {
-  color: #C70D2C;
-  text-decoration: none;
-  font-size: 1em;

[juliandoucette, 2017/10/08 13:18:12]

NIT: This doesn't actually do anything because em is relative. p of 1.25em > a
of 1em = 1.25em.

-  font-weight: 400;
-}
-
-p > a:hover {
+
+/*******************************************************************************
+ * #footer
+ ******************************************************************************/
+
+#footer
+{
+  overflow: auto;
+  margin-top: 2em;
+  padding: 2em 0em;
+  color: #ececec;
+  background-color: #292929;
+  font-size: 0.9em;
+}
+
+/* #footer body
+ ******************************************************************************/
+
+#footer h5
+{
+  margin-top: 1em;
+  margin-bottom: 1.5em;
+}
+
+#footer h5:after

[Thomas Greiner, 2018/02/16 16:19:16]

Detail: The standard way to write this is `#footer h5::after`. Unlike Websites,
we don't need to support older browsers who don't understand the standard syntax
yet.

See also https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements

+{
+  content: "";
+  display: block;
+  width: 1.25em;
+  height: 0.125em;
+  margin-top: 0.5em;
+  background-color: #ececec;
+}
+
+#footer ul
+{
+  padding: 0em;
+  list-style-type: none;
+}
+
+#footer a
+{
+  color: #ececec;
+}
+
+#footer a:hover,
+#footer a:active,
+#footer a:focus
+{
+  color: #fff;
   text-decoration: underline;
 }
 
-/*******************************************************************************
- * Grid component

[juliandoucette, 2017/10/08 13:18:11]

Now that I think about it, you could include all of website-defaults main.css
(suggest "defaults.css") instead of inserting this component into this CSS file.
[the performance impact should be negligible, it should make updating easier, it
should reduce maintenance cost e.g. if you change the grid component like you
did with .column box-sizing below (which is normally applied by website-default
base styles)]. I doubt that we will introduce npm & scss into adblockplusui
anytime soon :(

- ******************************************************************************/
-/**
- * - .row contains one or more .column(s)
- * - .row clears .column(s)
- * - .row negates the left & right padding of it's left-most & right-most
- *   .column(s) while preserving consistent padding between .column(s)
- */
- .row {
-  margin: 0px -1em; }
-
-.row:after {
-  display: block;
-  clear: both;
-  content: ""; }
-
-/**
- * - .column is 100% width by default
- * - Modifier classes are applied to .column to change it's width
- * - Modifier classes behave differently on different device widths
- */
-.column {
-  position: relative;
-  width: 100%;
-  min-height: 1px;
-  padding: 1em;
-  box-sizing: border-box;
-}
-
-/* - .column(s) within .row .reverse appear in reverse order
- * - .column(s) within [dir=rtl] appear in reverse order respectively
- */
-.column,
-[dir="rtl"] .reverse .column {
-  float: left; }
-
-.reverse .column,
-[dir="rtl"] .column {
-  float: right; }
-
-@media (min-width: 768px) {
-  .one-half,
-  .one-fourth {
-    width: 50%; } }
-
-@media (min-width: 992px) {
-  .one-third {
-    width: 33.333333%; }
-  .two-thirds {
-    width: 66.666667%; } }
-
-@media (min-width: 992px) {
-  .one-fourth {
-    width: 25%; }
-  .three-fourths {
-    width: 75%; } }
-
-@media (max-width: 1194px) {
-  .one-third {
-    width: 100%;
-  }
-}
-
-@media (max-width: 1194px) {
-  .column {
-    padding: 4em 0;
-    border-bottom: 1px solid lightgrey;
-  }
-}
-
-
-    
+/* #footer #social-list
+ ******************************************************************************/
+
+#social-list ul
+{
+  /* negative margin canceled out by li margin below */
+  margin: 0em -0.375em;
+}
+
+#social-list li
+{
+  float: left;
+  margin: 0em 0.375em;
+}
+
+[dir="rtl"] #social-list li
+{
+  float: right;
+}
+
+#social-list img
+{
+  height: 2em;
+}
+
+@media(min-width: 1200px)
+{
+  #social-list img
+  {
+    height: 3em;
+  }
+}
+
+/* #footer #footer-legal
+ ******************************************************************************/
+
+#footer-legal
+{
+  margin-top: 1em;
+}
+
+@media(min-width: 1200px)
+{
+  #footer-legal
+  {
+    margin-top: 2em;
+  }
+}
+
+/* #footer #footer-legal #legal-list
+ ******************************************************************************/
+
+#legal-list li
+{
+  float: left;
+}
+
+[dir="rtl"] #legal-list li
+{
+  float: right;
+}
+
+#legal-list li:after
+{
+  margin: 0em 0.375em;
+}
+
+/* IE9+ only */

[Thomas Greiner, 2018/02/16 16:19:19]

This is not relevant for us since we don't support IE9 so let's remove it.

+#legal-list li::after
+{
+  content: "|"
+}
+
+#legal-list li:last-of-type::after
+{
+  content: none;
+}
+
+/* float copyright and legal links left and right on ~desktop */
+@media(min-width: 1200px)
+{
+  #copyright-notice
+  {
+    float: left;
+  }
+
+  [dir="rtl"] #copyright-notice
+  {
+    float: right;
+  }
+
+  #legal-list
+  {
+    float: right;
+  }
+
+  [dir="rtl"] #legal-list
+  {
+    float: left;
+  }
+}
+
+/* hide all footer links except #social-list on ~mobile */
+@media(max-width: 767px)
+{
+  #footer .column
+  {
+    display: none;
+  }
+
+  /* overriding display:none above */
+  #footer #social-list
+  {
+    display: block;
+  }
+}