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/>.
  */
 
-/*!
- * This file contains parts of:
- * - normalize.css v5.0.0 | MIT License | github.com/necolas/normalize.css
- */
-
 /* UDL content styles
  *******************************************************************************
  * - Global
  * - Headings
  * - Body content
  * - Alignment
- * - Media queries
  ******************************************************************************/
 
 /* 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;

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

 }
 
 /**
  * Center content within a (responsive) fixed width
  */
 .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/15 14:28:56]

Why are we clearing the container ? It's not float as far as I can see.

[juliandoucette, 2016/11/16 14:58:51]

Acknowledged.

[juliandoucette, 2016/11/17 16:09:34]

Done.

-{
-  display: block;
-  clear: both;
-  content: "";
-}
-
-/**
- * Clear all floating elements
- */
-.clearfix

[juliandoucette, 2016/11/16 14:58:48]

NOTE: We will drop this class and re-add if needed.

[juliandoucette, 2016/11/17 16:09:38]

Done.

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

[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: $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: $desktop-width;
+  }
 }
 
 p,
 ol,
 ul,
 dl,
 pre,
 blockquote
 {
   /* Set consistent margins (opinionated) */
   margin: $small-space 0px;
 }
 
 /* Headings
  ******************************************************************************/
 
 h1,
 h2,
 h3,
 h4,
 h5,
 h6
 {
   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-size: round($large-font * 1.4);
 }
 
 h3
 {
   font-size: round($large-font * 1.2);
@@ skipping 12 matching lines @@
 h6
 {
   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;
-  top: -0.5em;

[saroyanm, 2016/11/15 14:28:55]

Why are we using EMs here ?
I assume this change is for "Normalization" reasons ?

[juliandoucette, 2016/11/16 14:58:50]

Acknowledged.

We will remove this in favor of vertical-align: super.

[juliandoucette, 2016/11/17 16:09:38]

Done.

   font-size: 75%;
-  /* Prevent `sub` and `sup` elements from affecting the line height */

[saroyanm, 2016/11/15 14:28:57]

Detail: It's only sup in this case, you can override both in reset CSS maybe ?

[juliandoucette, 2016/11/16 14:58:50]

Acknowledged.

We probably don't need line-height here.

[juliandoucette, 2016/11/17 16:09:35]

Done.

-  line-height: 0px;
-  vertical-align: baseline;

[saroyanm, 2016/11/15 14:28:56]

Why are we aligning to the baseline ? Doesn't looks like it provides any value.

[juliandoucette, 2016/11/16 14:58:48]

Acknowledged.

We will change this to vertical-align: super.

[juliandoucette, 2016/11/17 16:09:36]

Done.

+  vertical-align: super;
 }
 
 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) */

[saroyanm, 2016/11/15 14:28:55]

* I can't see default color being set.
* I'm not sure if we need this comment at all/
* I think we can align with designers to style hover state to at least make it
obvious that users are hovering a link (can be done separately from this review)

[juliandoucette, 2016/11/16 14:58:50]

Acknowledged.

I will remove and/or adjust the comment.

[juliandoucette, 2016/11/17 16:09:38]

Done.

   text-decoration: underline;
 }
 
 img
 {
-  margin: 0px $small-space / 2;
+  /* Make fixed width images responsive */
+  max-width: 100%;
   /* Remove the border on images inside links in IE 10-. */
   border-style: none;
 }
 
 hr
 {
-  border-top: 0px;
-  border-bottom: 2px solid $gray;
+  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;
-}
-
-pre
-{
-  overflow: auto;
-  padding: 0px $small-space;
-}
-
-code
-{
-  padding: 0px $small-space / 4;
-}
-
-pre,
-code
-{
-  background-color: $gray-light;
-  font-family: $monospace-font;
-  font-size: $medium-font;
-}
-
-pre code
-{
-  padding: 0px;
+  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 25 matching lines @@
   /* undo browser default (opinionated)*/
   font-weight: $bold-weight;
 }
 
 dd
 {
   margin: $small-space / 2 0px $small-space 0px;
 }
 
 /**
  * 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,
 {
   list-style-type: none;
 }
 
 [dir="ltr"] .unstyled-list
 {
   padding-left: 0px;
 }
 
 [dir="rtl"] .unstyled-list
 {
   padding-right: 0px;
 }
 
 [dir="ltr"] .unstyled-list .unstyled-list
 {
   padding-left: $medium-space;
 }
 
 [dir="rtl"] .unstyled-list .unstyled-list
 {
   padding-right: $small-space;
 }
 
-audio,

[saroyanm, 2016/11/15 14:28:57]

I think we only need this styles in case we will host videos/audios on our side,
but whhile we are using 3-rd party websites that ex. youtube which we are
loading in the Iframe this styles do not have an effect.

[juliandoucette, 2016/11/16 14:58:49]

Acknowledged.

Agreed.

[juliandoucette, 2016/11/17 16:09:34]

Done.

-video
-{
-  /* undo browser default (opinionated) */
+/* Alignment
+ ******************************************************************************/
+
+.full-width
+{
   display: block;
-}
-
-/**
- * Embed 16x9 iframe video
- */
-.embed
-{
-  display: block;
-  position: relative;
-  overflow: hidden;
-  padding: 0% 0% 56.25% 0%;
-}
-
-.embed iframe
-{
-  position: absolute;
-  top: 0px;
-  left: 0px;
-  width: 100%;
-  height: 100%;
-  /* undo browser default */
-  border: 0px;
-}
-
-/* Alignment
- ******************************************************************************/
-
-/**
- * Clear and stretch element full-width
- */
-.full-width
-{
-  display: block;
-  clear: both;

[saroyanm, 2016/11/15 14:28:56]

Why are we clearing the full width elements ? 
I thought we are planing to control the clearing behavior on the floating
elements ex. setting up :after element, so I can't see a reason of using the
clear on the elements that suppose to be just a full width elements, or am I
missing smth ?

[juliandoucette, 2016/11/16 14:58:49]

Acknowledged.

We can handle this edge case with inline CSS or clearfix depending on how often
we run into it.

[juliandoucette, 2016/11/17 16:09:36]

Done.

-  overflow: auto;

[saroyanm, 2016/11/15 14:28:55]

Why do we need to set the overflow value here ?
I can't see a reason why the default value is not enough for this element ?

[juliandoucette, 2016/11/16 14:58:49]

Acknowledged.

This addresses an edge case where we put floating elements inside of a
full-width container. We will address this edge case individually instead of in
UDL.

[juliandoucette, 2016/11/17 16:09:35]

Done.

   width: 100%;
   margin: $small-space 0px;
 }
-
-.float-start,
-.float-end
-{
-  display: block;
-  margin: 0px 0px $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;
-}
-
-/* Media queries
- ******************************************************************************/
-
-@media(min-width: $mobile-breakpoint)

[saroyanm, 2016/11/15 14:28:56]

Seems like we started to support 4 different screen sizes in comparison to old
3:
1. Is this change intentional ?
2. I think if we support 4 sizes we need to change the naming, because
$mobile-breakpoint and $desktop-breakpoint behave inconsistently IMO.

[juliandoucette, 2016/11/16 14:58:50]

Acknowledged.

We will remove mobile-breakpoint and keep mobile-width.

[juliandoucette, 2016/11/17 16:09:35]

Done.

-{
-  .container
-  {
-    width: $mobile-width;
-  }
-}
-
-@media(min-width: $tablet-breakpoint)
-{
-  .container
-  {
-    width: $tablet-width;
-  }
-}
-
-@media(min-width: $desktop-breakpoint)
-{
-  .container
-  {
-    width: $desktop-width;
-  }
-}