Issue 4607 - Default content styles

scss/_content.scss

 /*!
  * This file is part of universal-design-language
  * Copyright (C) 2016 Eyeo GmbH
  *
  * universal-design-language is free software: you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as published by
  * the Free Software Foundation, either version 3 of the License, or
  * (at your option) any later version.
  *
  * universal-design-language 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 web.starter-kit.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-/** UDL content styles */
-
-/*!
- * This file contains parts of:
- * - normalize.css v5.0.0 | MIT License | github.com/necolas/normalize.css

[saroyanm, 2016/11/10 16:30:44]

I thought we were planing to move parts of normalize.css into reset.css and and
rename the file if needed. Or create separate normalize.css

[juliandoucette, 2016/11/10 17:42:07]

We split parts of normalize.css into _reset.scss and _content.scss. That's why
"this file contains parts of normalize.css". We have to give attribution to
comply with the MIT license.

- */
-
-/* Table of contents
+/* UDL content styles
  *******************************************************************************
- * - Document
+ * - Global
  * - Headings
- * - Block elements
- *   - Blockquotes
- *   - Code blocks
- *   - Lists
- *   - Unstyled lists
- *   - Embeds
- * - Inline elements
- *   - Links
- *   - Images
+ * - Body content
  * - Alignment
  ******************************************************************************/
 
-/* Document
+/* Global

[juliandoucette, 2016/11/22 00:54:04]

@Manvel can you also acknowledge and review the requirements to verify that I am
not forgetting something that should / shouldn't be in this component?

[saroyanm, 2016/11/24 13:26:51]

I've noticed that the ticket is out of sync, ex. we removed pre and code blocks,
considering to implement them in future. 
For the initial push I think this file looks good, we can add more styles as
soon we will align with designers, I think they also will review it as soon we
will have this ready.
Long story short: Let's move forward and add components as soon we will need
them in current file.

[saroyanm, 2016/11/24 15:18:42]

Acknowledged.

We will move forward and add more component by need.

[juliandoucette, 2016/11/25 16:28:43]

Done.

  ******************************************************************************/
 
 html
 {
-  font-family: $sans-font;
+  font-family: $primary-font;
   font-size: $medium-font;
   line-height: 1.15;

[saroyanm, 2016/11/10 16:30:43]

Detail: please specify units where possible.

[juliandoucette, 2016/11/10 17:42:07]

Acknowledged.

[juliandoucette, 2016/11/11 16:27:10]

MDN prefers unit-less numbers for line-height values.

@see
https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#Prefer_unitless_...

I recommend we leave this as-is.

[juliandoucette, 2016/11/22 00:54:02]

Note: I think I should set the primary foreground and background colors here.

Thoughts @Manvel?

[saroyanm, 2016/11/24 13:26:52]

Agree.

[juliandoucette, 2016/11/25 16:28:42]

Done.

 }
 
 /**
- * A centered, responsive, fixed-width container with small padding
+ * Center content within a (responsive) fixed width
  */
-.container,
-/**
- * A full-width container with small padding

[saroyanm, 2016/11/10 16:30:43]

The way the comments are written are affecting CSS file readability IMO.
I'll strongly recommend if there is a need for the comment to put it on top of
the group of selectors.
We can have this comments moved to HTMl file. We can have general comment on top
of both, ex.: "responsive container"

[juliandoucette, 2016/11/10 17:42:08]

Acknowledged.

[juliandoucette, 2016/11/11 16:27:11]

Done.

- I removed these comments (I don't think we need them).
- I removed .container-fluid (I don't think we need it).

- */
-.container-fluid
-{
-  width: 540px;

[saroyanm, 2016/11/10 16:30:45]

I thought the plan was to calculate the width from the breakpoint values.

[juliandoucette, 2016/11/10 17:42:07]

No. There is no algorithm to calculate these widths. They are based on browser
statistics.

[juliandoucette, 2016/11/11 16:27:14]

I have included ${device}-${width} variables in _variables.scss instead.

+.container

[juliandoucette, 2016/11/22 00:54:04]

I'm considering moving this to _reset.scss and renaming _reset.scss to
_base.scss because there are few things being "reset" in _reset.scss and a
"container" is not "content".

Thoughts @Manvel?

[saroyanm, 2016/11/24 13:26:50]

I think we might need in future separate file for reset and normalization, but
I'm fine with it, make sense to separate them in the file. I'll be fine also
having reset and base separately. 
Don't feel strong regarding both solutions.
Also make sense to move ".full-width" class I think and "html" selector.

[saroyanm, 2016/11/24 15:18:44]

Acknowledged.

We will leave here only "markdown" related rules and move reset/normalization
related general and base rules to the _base.scss.

[juliandoucette, 2016/11/25 16:28:41]

Done.

+{
+  width: $mobile-width;
   max-width: 100%;
   margin: 0px auto;
   padding: 0px $small-space;
 }
 
-.container:after,

[saroyanm, 2016/11/10 16:30:44]

I do not think that this rules belong to current review, are they ? Looks like
grid specific implementation if I'm not wrong.

[juliandoucette, 2016/11/10 17:42:09]

I think you are wrong. These styles clear containers and any element that
contains the clearfix class. This applies to the grid and any other floating
elements (EG: float-start, float-end).

[juliandoucette, 2016/11/11 16:27:14]

Done.

I separated these classes and added more explanation.

-.container-fluid:after,
-/**
- * Force any element to clear all
- */
-.float:after

[juliandoucette, 2016/11/10 16:08:07]

This is a mistake. ".float" should be ".clearfix".

[juliandoucette, 2016/11/11 16:27:13]

Done.

-{
-  display: block;
-  clear: both;
-  content: "";
-}
-
 @media(min-width: $tablet-breakpoint)

[saroyanm, 2016/11/10 16:30:46]

Note: I do see a reason why @media queries are located after the .container
styles, but as soon we will add more rules into @media queries I think it would
be more convinient to move them on the bottom of the file. Weak suggestion: we
can move them to the bottom now.

[juliandoucette, 2016/11/10 17:42:08]

Don't you think that the rules that are applied to .container are more readable
if they are kept together?

[juliandoucette, 2016/11/11 16:27:14]

Done.

Looks like you and Thomas outnumber me on this one.

[saroyanm, 2016/11/21 18:44:39]

I thought we decided to move this rules to the bottom:
https://codereview.adblockplus.org/29361647/diff2/29362186:29363507/scss/_con...

[juliandoucette, 2016/11/22 00:54:04]

Acknowledged.

I shouldn't have moved this back.

[juliandoucette, 2016/11/25 16:28:43]

Done.

 {
   .container
   {
-    width: 720px;
+    width: $tablet-width;

[saroyanm, 2016/11/21 18:44:40]

What about calculating container width according to the breakpoint value ? I
thought we agreed to do so. 

[juliandoucette, 2016/11/22 00:54:03]

No. 

I said before that:

- The breakpoints are exact numbers based on common device sizes
- The widths are rounded numbers that fit comfortably within the breakpoint
sizes

How would we calculate the widths without using SCSS methods like round (which
we are trying to avoid?) or subtracting very specific numbers (which are
invalidated if we change the breakpoints)?

[saroyanm, 2016/11/24 13:26:50]

I was suggestion something like -> "$tablet-breakpoint - 48px"

[saroyanm, 2016/11/24 15:18:43]

Acknowledged.

We will keep variables and not calculate according to breakpoints [designers
need to be on charge of changing them]

[juliandoucette, 2016/11/25 16:28:42]

Done.

   }
 }
 
 @media(min-width: $desktop-breakpoint)
 {
   .container
   {
-    width: 960px;
+    width: $desktop-width;
   }
 }
 
-h1,
-h2,
-h3,
-h4,
-h5,
-h6,
 p,
 ol,
 ul,
 dl,
 pre,
 blockquote
 {
   /* Set consistent margins (opinionated) */
   margin: $small-space 0px;
 }
 
 /* Headings
  ******************************************************************************/
 
 h1,

[saroyanm, 2016/11/10 16:30:46]

Maybe it's a personal opinion, but I kinda feel like the overwriting the rules
doesn't make the file more readable, what about removing headings from top
selectors and having only here, by changing the rule to:
margin: $medium-space 0px $small-space 0px;

[juliandoucette, 2016/11/10 17:42:06]

Acknowledged.

[juliandoucette, 2016/11/11 16:27:12]

Done.

 h2,
 h3,
 h4,
 h5,
 h6
 {
-  margin-top: $medium-space;
-}
-
-h1,
+  margin: $medium-space 0 $small-space 0;
+}
+
+h1
+{
+  font-size: round($large-font * 1.6);

[saroyanm, 2016/11/21 18:44:36]

I would avoid using SASS methods(like round) until it's really needed.

[saroyanm, 2016/11/21 18:44:40]

Thought: I think you already mentioned that and I want to stress that topic
again, what you think if we will start using more accessible measures (like ems)
for the fonts-sizes as you suggested while ago ?
That I assume may also solve the problem of multiplying and using round methods
here I think.

[juliandoucette, 2016/11/22 00:54:03]

The idea here was to base the larger font sizes on $large-font size. This is not
possible if we want to avoid using round().

I recommend using rem and a polyfill (for IE8 only, like we do already for media
queries, HTML5 tags, and other JavaScript features on some websites): 
https://github.com/chuckcarpenter/REM-unit-polyfill

[saroyanm, 2016/11/24 13:26:49]

What about having the floating pixel values ? Anyway if we will use relative
approach we will not need the rounding anymore.
* REMs are not completely supported till IE11 and apparently some limitation for
Chrome for Android.
* It mostly depends on the approach why you think we need REMs ? They act
differently and mostly depends on the approach we are trying to propose.
* With rems you can't change the font-size of child elements relatively by
assigning font-size to the parent.
* I think EMs provide more flexibility in this case

My suggestion was about improving accessibility and for that reason Even if we
will use 1 of two approaches we need to remove fixed font-size from HTML
element.

[saroyanm, 2016/11/24 15:18:43]

Acknowledged.
We will use Ems for everything content related, but keep away from the
components.

[juliandoucette, 2016/11/25 16:28:44]

Done.

Except for line-height because of
https://developer.mozilla.org/en/docs/Web/CSS/line-height#Prefer_unitless_num...

+}
+
 h2
 {
-  font-weight: $thin-weight;
-}
-
-h1
-{
-  font-size: 48px;
-}
-
-h2
-{
-  font-size: 36px;
+  font-size: round($large-font * 1.4);
 }
 
 h3
 {
-  font-size: 24px;
+  font-size: round($large-font * 1.2);
 }
 
 h4
 {
-  font-size: 20px;
+  font-size: $large-font;
 }
 
 h5
 {
-  font-size: 16px;
+  font-size: $medium-font;
 }
 
 h6
 {
-  font-size: 14px;
-}
-
-/* Block elements
- ******************************************************************************/
+  font-size: $small-font;
+}
+
+/* Body content
+ ******************************************************************************/
+
+abbr
+{
+  text-decoration: underline dotted;

[saroyanm, 2016/11/21 18:44:36]

Apparently shorthand properties are only supported in Firefox and Safari ->
https://developer.mozilla.org/en/docs/Web/CSS/text-decoration 

[juliandoucette, 2016/11/22 00:54:03]

Acknowledged.

[juliandoucette, 2016/11/25 16:28:43]

Done.

+  cursor: help;
+}
+
+b,
+strong
+{
+  font-weight: $bold-weight;
+}
+
+small
+{
+  font-size: $small-font;
+}
+
+sup
+{
+  position: relative;
+  font-size: 75%;
+  vertical-align: super;
+}
+
+a,
+a:visited
+{
+  color: $accent;
+  /* Remove the gray background on active links in IE 10. */
+  background-color: transparent;
+  text-decoration: none;
+}
+
+a:hover,
+a:active,
+a:focus
+{
+  text-decoration: underline;
+}
+
+img
+{
+  /* Make fixed width images responsive */
+  max-width: 100%;
+  /* Remove the border on images inside links in IE 10-. */
+  border-style: none;
+}
 
 hr

[saroyanm, 2016/11/10 16:30:46]

Detail: In HTML "hr" element is missing.

[juliandoucette, 2016/11/10 17:42:09]

Acknowledged.

[juliandoucette, 2016/11/11 16:27:11]

Done.

Added.

 {
-  /* Add the correct box sizing in Firefox. */

[saroyanm, 2016/11/10 16:30:45]

What is the problem with border-box reset we have in Firefox ?

[juliandoucette, 2016/11/10 17:42:06]

Acknowledged.

[juliandoucette, 2016/11/11 16:27:11]

Done.

-  box-sizing: content-box;
-  /* Show the overflow in Edge and IE. */
-  overflow: visible;

[saroyanm, 2016/11/10 16:30:44]

Not really sure why we need overflow property on hr element ?

[juliandoucette, 2016/11/10 17:42:10]

It depends on how we style hr. The default style is overflow: visible except in
IE. Hence the normalize fix.

[juliandoucette, 2016/11/11 16:27:12]

Done.

Removed. These styles were not necessary for us.

-  height: 0px;

[saroyanm, 2016/11/10 16:30:45]

note: Feels like this style belongs to normalize.css

[juliandoucette, 2016/11/10 17:42:09]

I put the normalize.css things that apply to content in content.scss

[juliandoucette, 2016/11/11 16:27:12]

Most of the normalize.css changes are made irrelevant by our styles now. Let me
know if you find anything that smells.

-  border: 1px solid $gray;
-}
-
-/* Blockquotes */
+  border: 1px solid $secondary-light;
+}
 
 [dir="ltr"] blockquote
 {
   padding-left: $small-space;
-  border-left: 5px solid $gray;
+  border-left: 5px solid $secondary-light;
 }
 
 [dir="rtl"] blockquote
 {
   padding-right: $small-space;
-  border-right: 5px solid $gray;
-}
-
-/* Code blocks */
-
-pre
-{
-  overflow: auto;
-  padding: 0px $small-space;
-}
-
-code
-{
-  padding: 0px $small-space / 4;
-}
-
-pre,
-code
-{
-  background-color: $gray-light;
-  /* Correct odd font inheritance in all browsers. */

[saroyanm, 2016/11/10 16:30:45]

Detail: seems like this also part of normalization and couple of styles below
I've also noticed. Make sense to decide where we want to move normalization
related properties, I thought we decided to move to reset.css

[juliandoucette, 2016/11/10 17:42:08]

See comment above.

[juliandoucette, 2016/11/11 16:27:12]

Correction:

These comments are no longer valid because I changed the CSS from normalize.css.
Font-family and font-size are set inconsistently across browsers. Setting them
for pre and code corrects this.

-  font-family: $monospace-font;
-  /* Correct the odd font sizing in all browsers. */
-  font-size: $medium-font;
-}
-
-pre code
-{
-  padding: 0px;
-}
-
-/* Lists */
+  border-right: 5px solid $secondary-light;
+}
 
 [dir="ltr"] ol,
 [dir="ltr"] ul
 {
   padding-left: $small-space * 1.5;
 }
 
 [dir="rtl"] ol,
 [dir="rtl"] ul
 {
@@ skipping 23 matching lines @@
 {
   /* undo browser default (opinionated)*/
   font-weight: $bold-weight;
 }
 
 dd
 {
   margin: $small-space / 2 0px $small-space 0px;
 }
 
-/** Unstyled lists */
-
+/**
+ * Remove list style from vertical lists

[juliandoucette, 2016/11/27 21:45:36]

Note.

Removed .unstyled-list because it cannot be applied via Markdown with markdown
extensions.

EG:

- 1
- 2
- 3
{: .test }

Applies .test to the last list item, not the list.

I think we should implement this in a separate component like .embed.

+ */
 .unstyled-list,
-.unstyled-list ul
 {
   list-style-type: none;
 }
 
 [dir="ltr"] .unstyled-list
 {
   padding-left: 0px;
 }
 
 [dir="rtl"] .unstyled-list
 {
   padding-right: 0px;
 }
 
-[dir="ltr"] .unstyled-list ul
+[dir="ltr"] .unstyled-list .unstyled-list
 {
   padding-left: $medium-space;
 }
 
-[dir="rtl"] .unstyled-list ul
+[dir="rtl"] .unstyled-list .unstyled-list
 {
   padding-right: $small-space;
 }
 
-/* Embeds */
-
-audio,
-video
-{
-  /* undo browser default (opinionated) */
+/* Alignment
+ ******************************************************************************/
+
+.full-width
+{
   display: block;
-}
-
-/** Responsive 16x9 YouTube video */
-
-.youtube-16x9

[saroyanm, 2016/11/10 16:30:44]

Why youtube ? I think this style shouldn't have specific website name. Previous
.embed were fitting better IMO.

[juliandoucette, 2016/11/10 17:42:09]

Acknowledged.

I thought it was simpler for content developers.

[juliandoucette, 2016/11/11 16:27:09]

Done.

I changed it back to .embed and added a comment.

-{
-  display: block;
-  position: relative;
-  overflow: hidden;
-  height: 0px;

[saroyanm, 2016/11/10 16:30:45]

I thought we decided to remove this property.

[juliandoucette, 2016/11/10 17:42:07]

Acknowledged.

[juliandoucette, 2016/11/11 16:27:14]

Done.

-  padding: 0% 0% 56.25% 0%;
-}
-
-.youtube-16x9 iframe
-{
-  position: absolute;
-  top: 0px;
-  bottom: 0px;

[saroyanm, 2016/11/10 16:30:44]

I thought we decided to skip this property.

[juliandoucette, 2016/11/10 17:42:06]

Acknowledged.

[juliandoucette, 2016/11/11 16:27:10]

Done.

-  left: 0px;
-  width: 100%;
-  height: 100%;
-  border: 0px;
-}
-
-/* Inline elements
- ******************************************************************************/
-
-abbr[title]

[saroyanm, 2016/11/10 16:30:43]

I wonder if there will be cases when we will use abbr element without title
attribute ?

[juliandoucette, 2016/11/10 17:42:06]

Acknowledged.

This came from normalize.

[juliandoucette, 2016/11/11 16:27:10]

Done.

-{
-  /* Remove the bottom border in Firefox 39-. */

[saroyanm, 2016/11/10 16:30:43]

We do support 48.0+ I think make sense to only concentrate on the platforms we
are supporting and add the minimal requirements in case we really need to
support old versions.
Reference: https://adblockplus.org/requirements

[juliandoucette, 2016/11/10 17:42:08]

Acknowledged.

I could have used this link a long time ago :D...

[juliandoucette, 2016/11/11 16:27:13]

Done.

-  border-bottom: none;
-  /* Add the correct text decoration in Chrome, Edge, IE, Opera, and Safari. */
-  text-decoration: underline dotted;
-  cursor: help;
-}
-
-b,
-strong
-{
-  font-weight: $bold-weight;
-}
-
-small
-{
-  font-size: $small-font;
-}
-
-sup
-{
-  position: relative;
-  top: -0.5em;
-  /* Prevent `sub` and `sup` elements from affecting the line height in

[saroyanm, 2016/11/10 16:30:46]

detail: I think this comment should refer to the line below.

[juliandoucette, 2016/11/10 17:42:08]

Acknowledged.

It does, according to normalize.css :/

[juliandoucette, 2016/11/11 16:27:10]

Done.

I moved and shortened the comment.

-   all browsers. */
-  font-size: 75%;
-  line-height: 0px;
-  vertical-align: baseline;
-}
-
-/* Links */
-
-a,
-a:visited
-{
-  /* Set default color and decoration (opinionated) */
-  color: $accent;
-  /* Remove the gray background on active links in IE 10. */
-  background-color: transparent;
-  text-decoration: none;
-}
-
-a:hover,
-a:active,
-a:focus
-{
-  /* Set default color and decoration (opinionated) */
-  text-decoration: underline;
-}
-
-/* Images */
-
-img
-{
-  margin: 0px $small-space / 2;
-  /* Remove the border on images inside links in IE 10-. */
-  border-style: none;
-}
-
-/* Alignment
- ******************************************************************************/
-
-.block,
-.float-start,
-.float-end
-{
-  display: block;
-  margin: 0px 0px $small-space 0px;
-}
-
-/** Display full-width */
-
-.block
-{
-  display: block;
-  clear: both;
-  overflow: auto;
   width: 100%;
   margin: $small-space 0px;
 }
-
-/** Float to start-of-line */
-
-[dir="ltr"] .float-start
-{
-  float: left;
-  margin-right: $small-space;
-}
-
-[dir="rtl"] .float-start
-{
-  float: right;
-  margin-left: $small-space;
-}
-
-/** Float to end-of-line */
-
-[dir="ltr"] .float-end
-{
-  float: right;
-  margin-left: $small-space;
-}
-
-[dir="rtl"] .float-end
-{
-  float: left;
-  margin-right: $small-space;
-}