Clean up the "elements" component in Bartik

Experimenting with a good way to review the existing code in the file.

Sweet! If I upload the css file with a .patch suffix then I can review the code with Dreditor.

1. /* ---------- Overall Specifications ---------- */
   

   Change this to @file comment, see [#1887862]

2.   margin: 0;
     padding: 0;
     border: 0;
   

   I think we can remove these lines, as they should be handled by normalize.css

3. .field-type-image a,
   h1 a,
   h2 a,
   [role*=banner] a,
   .feed-icon,
   .contextual-links a,
   .toolbar a {
     border-bottom: none !important;
   }
   

   This kind of selector group is against one of the SMACSS principles which is to keep all styling affecting one component in one place. All these selectors should be moved to the correct file.

4. h1,
   h2,
   h3,
   h4,
   h5,
   h6 {
     margin: 1.0em 0 0.5em;
     font-weight: inherit;
   }
   h1 {
     font-size: 1.357em;
     color: #000;
   }
   h2 {
     font-size: 1.143em;
   }
   h3 {
     font-size: 1.092em;
   }
   h4 {
     font-size: 1.05em;
   }
   h5 {
     font-size: 0.889em;
     text-transform: uppercase;
     letter-spacing: 0.1em;
   }
   h6 {
     font-size: 0.67em;
     text-transform: uppercase;
     letter-spacing: 0.1em;
   }
   

   In Seven, we've added .header-a,.header-b,.header-c,.header-d,.header-e classes that match up with the equivalent header styles. Feels like we should add them here as well.

5. /* ------------------ Reset Styles ------------------ */
   

   Remove this, these aren't reset styles

6. a.feed-icon {
     display: inline-block;
     padding: 15px 0 0 0;
   }
   

   This should go into it's own component file

Having a look at this now.

I don't think the standalone CSS file in #3 is current enough any more to just drop in and work from it: 8.0.x is too much of a moving target. For example, just dropping it in loses the fonts!

With that in mind, I've uploaded a patch/diff file as a starting point for discussion instead. I've tried to follow as many as possible of the recommendations here, but it would be good if it could be reviewed critically by those who know SMACSS, BEM and core standards better than me.

A lot of the changes between #3 and HEAD seem to have come from other Bartik tidyups, so I mention them below to indicate that I've not omitted them just because I felt like it:

• 1. html { height: 100%; } and body { min-height: 100% } added in a6893b41 #2398451: Clean up "layout" CSS in Bartik.
• 2. font settings added in 82fa1a65 #2398447: Remove the "typography" CSS file in Bartik.
• 3. .heading-a etc. added in 2da79506 #2422381: elements.css css lint fix bartik, but I added a full stop to the comment!
• 4. RTL comments added in dff56880 #2396489: Add missing RTL rules to Bartik theme CSS.

Anyway, comments welcome.

Unassigning. I knew there was a select dropdown I'd forgotten to change :)

The patch in #6 no longer cleanly applies as 8.0.x is a moving target. I've re-rolled against HEAD and attached a patch.

These seem to be quite small changes: can anyone see anything wrong with them that might block them?

@jp.stacey Everything looks fine in patch :)

Adding Description of file in header.css

/**
 * @file
 * Header styles for Bartik.
 */

@Manjit thanks for reviewing the patch and confirming it applies.

Regarding your addition of the header.css @file comment. I've just opened #2466983: Clean up the "header" component in Bartik to track changes to header.css and added a mention of your suggestion. That other issue needs improving to use the standard template, but for now it's a good placeholder for your suggestion.

This issue should ideally only track changes to elements.css (unless those changes entail changes to other files, to make sure we don't lose styling by deleting it.)

Regarding your addition of the header.css @file comment. I've just opened #2466983: Clean up the "header" component in Bartik to track changes to header.css and added a mention of your suggestion.

Yep that sounds like a good idea, we should keep changes focused on one file per issue.

+++ b/core/themes/bartik/css/base/elements.css
@@ -28,19 +28,14 @@ a:focus,
+h2 a {
+  border-bottom: none important;

+++ b/core/themes/bartik/css/components/content.css
@@ -154,6 +154,9 @@ h1#page-title {
+.field-type-image a {
+  border-bottom: none !important;

+++ b/core/themes/bartik/css/components/contextual.css
@@ -1,8 +1,14 @@
 .contextual-region .contextual .contextual-links a {
+  border-bottom: none !important;

I'm unsure that these !important properties are required and I think we should avoid them if we can.

We can also run csslint against this file to see if it passes. The easiest way to run this test is using: https://github.com/lewisnyman/drupalcore-frontend-toolkit

@LewisNyman thanks for that: looking at your feedback now.

This is still work in progress, but the patch and interdiff attached details improvements so far.

To test why !important declarations were required, I set up an image field on basic page, which is display-formatted to link to the original image. I created some content, with an image, and set it to be promoted to the front page.

The following !important are all intended to remove border-underline from elements but are NOT REQUIRED:

• elements.css: h2 a (linked teaser titles on e.g. the front page)
• content.css: .field-type-image a (linked images on e.g. the node page itself)
• contextual.css: .contextual-region .contextual .contextual-links a (contextual links e.g. quickedit links)
• feed-icon.css: .feed-icon (RSS feed icon on e.g. the front page)
• header.css: [role*=banner] a (links in the page header element e.g. top-right menu links)
• toolbar.css: .toolbar a (links in the admin toolbar)

I've therefore removed these. I've also removed an extra !important in contextual.css, in the same selector as the declaration added as part of this ticket: the rule removes text-shadow from those links, but still works without it.

Finally, I've also checked elements.css for other !important declarations, and there are none. Hooray!

I'll now look at running CSSLint over elements.css and any files created by the patch.

Settings to needs review so we can get some feedback

To confirm, @LewisNyman has CSSLint'ed the relevant files (discussed on IRC) and they're looking good. Also attempting with this comment to change my attribution to include my company :)

I'm going over the element.css file for more improvements we can make

blockquote:before {
  color: #bbb;
  content: "\201C";
  font-size: 3em;
  line-height: 0.1em;
  margin-right: 0.2em; /* LTR */
  vertical-align: -.4em;
}

We should have vertical-align: -0.4em

blockquote:after {
  color: #bbb;
  content: "\201D";
  font-size: 3em;
  line-height: 0.1em;
  vertical-align: -.45em;
}

We should have vertical-align: -0.45em

img {
  max-width: 100%;
  height: auto;
  border: 0;
}

Maybe we don't need border: 0? It feels like that would be covered in normalise

a {
  text-decoration: none;
  border-bottom: 1px dotted;
}

I think this selector also needs the .link class as we include it in the css below.

body {
  min-height: 100%;
  line-height: 1.5;
  word-wrap: break-word;

I'm unsure why we have word-wrap: break-word up here. Maybe we should leave it to be sure but I've never seen this in a body tag before

Done:

* -.4em => -0.4em
* -.45em => -0.45em
* img { border: 0; } (indeed covered by normalize.css, so removed)
* a { ... } (included a.link along with a)

The remaining item:

* body { word-wrap: break-word}

- I think we should leave this as it is. It's a way of preventing long URLs from breaking layout, and it was in the old bartik styles.css back in December when SMACSS was applied (e6afbff, #2375673: Split Bartik's CSS into SMACSS style components). So it has a pedigree, which isn't quite the same as saying it adds value ;)

Find patch/interdiff attached.

Sounds good to me, I think this issue could benefit from some visual regression testing because it affects so many elements on so many pages

I ran the patch through siteeffect, checked up on 450 urls and couldn't find any visual changes.

Added the blank line after the file comment as asked in #22

Committed 5a46466 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

+++ b/core/themes/bartik/css/base/elements.css
@@ -28,19 +30,14 @@ a:focus,
-  border-bottom: none !important;

+++ b/core/themes/bartik/css/components/contextual.css
@@ -1,10 +1,16 @@
-  text-shadow: 0 0 0 !important;

Less importance!!! nice :)