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
+ *******************************************************************************
+ * - Document
+ * - Headings
+ * - Block elements
+ *   - Blockquotes
+ *   - Code blocks
+ *   - Lists
+ *   - Unstyled lists
+ *   - Embeds
+ * - Inline elements
+ *   - Links
+ *   - Images
+ * - Alignment
+ ******************************************************************************/
+
+/* Document
+ ******************************************************************************/
+
+html
+{
+  font-family: $sans-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.

+}
+
+/**
+ * A centered, responsive, fixed-width container with small padding
+ */
+.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.

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

+{
+  .container
+  {
+    width: 720px;
+  }
+}
+
+@media(min-width: $desktop-breakpoint)
+{
+  .container
+  {
+    width: 960px;
+  }
+}
+
+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,
+h2
+{
+  font-weight: $thin-weight;
+}
+
+h1
+{
+  font-size: 48px;
+}
+
+h2
+{
+  font-size: 36px;
+}
+
+h3
+{
+  font-size: 24px;
+}
+
+h4
+{
+  font-size: 20px;
+}
+
+h5
+{
+  font-size: 16px;
+}
+
+h6
+{
+  font-size: 14px;
+}
+
+/* Block elements
+ ******************************************************************************/
+
+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 */
+
+[dir="ltr"] blockquote
+{
+  padding-left: $small-space;
+  border-left: 5px solid $gray;
+}
+
+[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 */
+
+[dir="ltr"] ol,
+[dir="ltr"] ul
+{
+  padding-left: $small-space * 1.5;
+}
+
+[dir="rtl"] ol,
+[dir="rtl"] ul
+{
+  padding-right: $small-space * 1.5;
+}
+
+li
+{
+  margin: $small-space / 2 0px;
+}
+
+ol ol,
+ul ul,
+ol ul,
+ul ol
+{
+  /* prevent double spacing lists */
+  margin: 0px;
+}
+
+ol ol
+{
+  list-style-type: lower-alpha;
+}
+
+dt
+{
+  /* undo browser default (opinionated)*/
+  font-weight: $bold-weight;
+}
+
+dd
+{
+  margin: $small-space / 2 0px $small-space 0px;
+}
+
+/** Unstyled lists */
+
+.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
+{
+  padding-left: $medium-space;
+}
+
+[dir="rtl"] .unstyled-list ul
+{
+  padding-right: $small-space;
+}
+
+/* Embeds */
+
+audio,
+video
+{
+  /* undo browser default (opinionated) */
+  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;
+}