Clean up the "content" component in Bartik

While working for #2398461: Clean up the "comments" component in Bartik I noticed that styles for unpublished comments where both in comments.css and content.css. So removed it from content.css.

A patch that is doing only that.

Just to test request

Took a quick look inside of current content.css file. Content as a name is way too generic and the file should be splitted up.

On lines 19-44 we have stuff that is related to view mode teaser in all content types. I would split this one up to it's own file.
On lines 61-91 we have stuff that is related to taxonomy term reference field. I'd put it either to it's own file or then I'd create a new file for all field-related styles.
On lines 117-177 we have node-preview related stuff which should be in already existing component called node-preview. Didn't make a complete check but it looks like most of it already is there.

Rest of the stuff is mostly .node__meta, .node__links, .node--unpublished etc node-related so for that I might create one file called node.css.

I will take a look on template a bit later.

#content has mentions in 6 different css-files:
1. admin.css
.path-admin #content image
2. content.css
#content h2
3. form.css
#content h2.comment-form
4. layout.css
50 different mentions
5. maintenance-page.css
.maintenance-page #content .section
6. print.css
.one-sidebar #content
.two-sidebars #content

I'm not sure if these should be handled together with this issue, but those mostly look like things we should get rid of.

By looking to page.html.twig I think that

        <main id="content" class="column" role="main">
          <section class="section">
            {% if page.highlighted %}<div id="highlighted">{{ page.highlighted }}</div>{% endif %}
            <a id="main-content" tabindex="-1"></a>
            {{ title_prefix }}
            {% if title %}
              <h1 class="title" id="page-title">
                {{ title }}
              </h1>
            {% endif %}
            {{ title_suffix }}
            {% if tabs %}
              <nav class="tabs" role="navigation" aria-label="{{ 'Tabs'|t }}">
                {{ tabs }}
              </nav>
            {% endif %}
            {{ page.help }}
            {% if action_links %}
              <ul class="action-links">{{ action_links }}</ul>
            {% endif %}
            {{ page.content }}
          </section>  <!-- /.section -->
        </main> <!-- /#content -->

That area is the one we should be looking for in this issue?

Worked a bit on this. Compared node-preview related stuff and it was already on node-preview.css, so I removed it. Changed the file name to node.css also.

Hello!
This is my input reg patch in #9. How about making the diff with git diff -C --staged after adding all files so that the history won't be lost?

Oh yeah my comment in #10 is about the renaming of content.css to node.css

Worked a bit on this.
1. Separated purely field-related stuff to field.css.
2. Moved ul.links to list.css.
3. Changed node__meta to node__metadata in node.html.twig and changed the css to match that change.
4. Changed h1-id to class and css to match that.
5. Added few comments to files.

Invalid library definition in core/themes/bartik/bartik.libraries.yml: Unexpected characters near ";" at line 16 (near "css/components/field.css: {};").

+++ b/core/themes/bartik/bartik.libraries.yml
@@ -10,11 +10,11 @@ global-styling:
+      css/components/field.css: {};

i.e. this. Should be an easy fix :)

Whoops. Fixed it.

Content.css has been changed since my last patch so it needs reroll.

page.html.twig has changed a bit so needed to reroll it. Also compiled multiple class attributes into one.

@lauriii It looks like we're missing the new node.css and field.css files?

+++ b/core/themes/bartik/templates/node.html.twig
@@ -83,7 +83,7 @@
-      <div class="node__meta">
+      <div class="node__metadata">

I would be careful about doing this just in Bartik. This should be consistent across of core

"node__meta" has one mention outside Bartik, it's in core/modules/node/templates/node.html.twig line 92 <footer class="node__meta">.

My current quess is that it's not consistent across of core, but no idea how to grep all possible options.

btw patch in #20 didn't move the content of content.css to node.css

Redid the patch with the files included

1. +++ b/core/themes/bartik/css/components/list.css
   @@ -65,3 +65,7 @@ ul.tips {
   +ul.links {
   +  color: #68696b;
   +  font-size: 0.821em;
   +}
   

   This styling is still in node.css. Is this a node specific thing? Maybe we should change the class to node__links?

2. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,94 @@
   +.content {
   +  margin-top: 10px;
   +}
   +.content__page-title {
   

   The content component should stay in the content.css file

3. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,94 @@
   +  line-height: 1;
   ...
   +  line-height: 1.4;
   ...
   +  line-height: 1.6;
   

   We always add em to the end of line height units.

4. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,94 @@
   +/* Is this actually targetting something? */
   

   Can we not leave these kinds of comments in the code? Conversation happens in the issue queue.

5. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,94 @@
   +.node__content {
   +  font-size: 1.071em;
   +  margin-top: 10px;
   +}
   +/* Visual styles for view mode teaser */
   +.node--view-mode-teaser .node__content {
   +  font-size: 1em;
   +}
   

   If we specify the full-content view mode gets the increased font size, then we won't have to reset it for the teaser, right?

6. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,94 @@
   +.node--unpublished,
   +.unpublished {
   

   Why do we have two classes? I don't think we use both

7. +++ b/core/themes/bartik/templates/page.html.twig
   @@ -126,12 +126,12 @@
   -              <h1 class="title" id="page-title">
   +              <h1 class="title" class="content__page-title">
   

   I think this makes more sense as just page-title. No reason for it to sit inside of component, also we have two class attributes now, I wonder if we can remove title....

My current quess is that it's not consistent across of core, but no idea how to grep all possible options.

I mean the system node.html.twig file, that has node__meta so if we are going to change the class we should do it there as well. Probably in another issue

Node--unpublished points to unpublished nodes. Unpublished points to unpublished comments. So that should probably go under comments component and I'd like to see it as node--comment__unpublished or something.

Concatenated common font-family selectors in three CSS files - content.css, header.css, tabs.css

Nice job @Amol,

A few things to consider:

- Don't forget to change issue status to "Needs review" in order to let testbot to test your patch and let reviews know that you submitted it.
- Your patch doesn't include the changes in the patch in comment #25: https://www.drupal.org/node/2398463#comment-9544083
- Read carefully the comments #26 and #27 and fix those suggestions.

Updated patch with #25 merged!

Nice work, @Amol. There are still a few things to do:

- In core/themes/bartik/css/components/content.css file I can see a few errors reported by CSSLint. For example the use of IDs and overqualified elements.

Please, check the errors in this list made by @lewisnyman: http://lewisnyman.co.uk/drupalcore-frontend-toolkit

Let me know if you need help.

It doesn't look like #25 + #28 to me. In #25 there's a file called field.css, things done in page.html.twig and bartik.libraries.yml.

Updated patch considering comments #31, #32

Updated patch with minor correction. Hoping this will pass the test!

@amolnw2778 are you sure you are working with the latest updates of Drupal?
The patch can't be applied because things have changed since the version of Drupal you're using.
Please git reset HEAD --hard and git pull origin 8.0.x before making a patch :)

+updates

ignore #41. Needs review

Fixes

@amolnw2778 When applying the patch the error lies in content.css, which means your content.css file is not up to date. It's also nice to post an interdiff after each change, see interdiff.

+1 content.css error fixes? hope so!

@amolnw2778 Great Job! I'll leave the reviewing to others :)

@tadityar this worked for me git fetch then git reset --hard origin/8.0.x but not yours, sorry! And thanks for the help :)

Thanks for the patches here! I think there are a few changes that need to be closer aligned to our CSS coding standards

1. +++ b/core/themes/bartik/css/components/content.css
   @@ -4,45 +4,26 @@
   +
   +/* @group Common font family */
   +
   +.node__meta,
   +.field-type-taxonomy-term-reference,
   +ul.links,
   +#page .ui-widget,
   +.content .ui-widget {
   +  font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
   

   I think we want to keep the font family in each component, it's important to keep all the styling for, see #2398447: Remove the "typography" CSS file in Bartik

2. +++ b/core/themes/bartik/css/components/content.css
   @@ -91,35 +71,14 @@ h1#page-title {
   -.node--unpublished,
    .unpublished {
   

   We want to keep the node---unpublished file, the unpublished class is the one that is too broad and we should remove that one

3. +++ b/core/themes/bartik/css/components/content.css
   @@ -139,10 +98,28 @@ ul.links {
   +
   +/* @group common background image selectors */
   +
   

   We don't usually use @group tags in CSS, can we use standards docblocks please?

4. +++ b/core/themes/bartik/css/components/content.css
   @@ -139,10 +98,28 @@ ul.links {
   +.node-preview-backlink,
   +.node-preview-backlink:focus,
   +.node-preview-backlink:hover,
   +.node-preview-backlink:active {
   +  background-image: url(../../../../misc/icons/000000/chevron-left.svg);
   +  background-repeat: no-repeat;
   +  background-position: left;
   +}
   

   I think we only need the one class here, as the focus/hover/active selectors will inherit from it'?

5. +++ b/core/themes/bartik/css/components/tabs.css
   @@ -1,18 +1,17 @@
   -div.tabs {
   -  font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
   -  margin-bottom: 20px;
   -}
   +div.tabs,
    .tabs ul.primary {
      font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
    }
   +div.tabs {
   +  margin-bottom: 20px;
   +}
   

   I'm confused, why are we removing the margin?

Considered @LewisNyman's points.

1. Separated font-family selectors. Gone through the link. But still recommends to use global font-family at one place and let it inherit for all child selectors. Wonder if we could have one global stylesheet file, which takes care of all repetitive properties.
2. Removed unpublished class and moved .node--unpublished to node.css. How about moving all ^.node selectors to node.css?
3. Cleaned.
4. Agreed! Cleaned.
5. Margin wasn't removed.

Nice job @Amol,

A few questions that maybe @lewisnyman can resolve:

+++ b/core/themes/bartik/bartik.libraries.yml
@@ -15,6 +15,7 @@ global-styling:
+      css/components/field.css: {}

I think we should remove this library dependency because the CSS file doesn't exist.

--- a/core/themes/bartik/bartik.libraries.yml
+++ b/core/themes/bartik/bartik.libraries.yml

@@ -15,6 +15,7 @@ global-styling:
       # @see https://www.drupal.org/node/2389735

@@ -22,6 +23,7 @@ global-styling:
+      css/components/node.css: {}

Same here.

+++ b/core/themes/bartik/css/components/content.css
@@ -139,10 +86,20 @@ ul.links {
+/* Grouped common background image selectors */
+.node-preview-backlink {
+  background-image: url(../../../../misc/icons/000000/chevron-left.svg);
+  background-repeat: no-repeat;
+  background-position: left;
+}
+[dir="rtl"] .node-preview-backlink {
+  background-image: url(../../../../misc/icons/000000/chevron-right.svg);
+  background-repeat: no-repeat;
+  background-position: right;
+}

We are not using /* LTR */ comment to indicate left-to-right styling anymore?

It still needs some work. You can see in the following screenshots that sidebar first blocks disappear and preview page is different (check preview button in the top):

Node BEFORE

Node AFTER

Preview BEFORE

Preview AFTER

Original idea was to get rid of content.css and replace it with node.css and field.css. Content as name is way too generic and its not a component.

Separated font-family selectors. Gone through the link. But still recommends to use global font-family at one place and let it inherit for all child selectors. Wonder if we could have one global stylesheet file, which takes care of all repetitive properties.

Ok, but this is undoing the work that we have just done in #2398447: Remove the "typography" CSS file in Bartik right? We are not in a situation where we can set one global font-family and let it inherit for everything, because we use two different font families for two different situations.

It's important that we try and make decisions that are inline with our CSS coding standards to keep our CSS consistent and predictable.

1. +++ b/core/themes/bartik/css/components/content.css
   @@ -139,10 +86,20 @@ ul.links {
   +/* Grouped common background image selectors */
   

   We need a full stop at the end of this comment.

2. +++ b/core/themes/bartik/css/components/content.css
   index 0d4cb58..439e6b4 100644
   --- a/core/themes/bartik/css/components/header.css
   
   +++ b/core/themes/bartik/css/components/header.css
   index d927c69..7a58c89 100644
   --- a/core/themes/bartik/css/components/tabs.css
   

   There are a few changes to other files that are not within the scope of this issue. Please try and keep all changes within the scope of this issue.

@Ruben #52:

1. Thanks for pointing out the preview issue. Its fixed!
2. node.css is used to take out all node
3. node.css, field.css not exist, but has in the previous patch flow, hence included. @LewisNyman your call.

@LewisNyman #56

1. Full stop at the end of comment
2. Excluded header.css and tabs.css, but kept node.css due to patch flow.

In @lauriiis patch #25 field.css had all field-spesific styles.

Thanks for the progress, we need to add the node.css and field.css files to the next patch.

1. +++ b/core/themes/bartik/css/components/content.css
   @@ -4,43 +4,15 @@
   -h1#page-title {
   +#page-title {
   

   This needs to be changed to a class not an ID.

2. +++ b/core/themes/bartik/css/components/content.css
   @@ -119,19 +72,10 @@ ul.links {
   -  background: #d1e8f5;
   -  background-image: -webkit-linear-gradient(top, #d1e8f5, #d3e8f4);
   -  background-image: linear-gradient(to bottom, #d1e8f5, #d3e8f4);
   +  background-color: #d1e8f5;
   +  background-color: -webkit-linear-gradient(top, #d1e8f5, #d3e8f4);
   +  background-color: linear-gradient(to bottom, #d1e8f5, #d3e8f4);
   

   We are setting the gradients on background color when they need to be set on background image, see the MDN documentation:

   The CSS linear-gradient() function creates an which represents a linear gradient of colors.

3. +++ b/core/themes/bartik/css/components/content.css
   @@ -139,10 +83,15 @@ ul.links {
    .node-preview-backlink {
      background-color: #419ff1;
   -  background: url(../../../../misc/icons/000000/chevron-left.svg) left no-repeat, -webkit-linear-gradient(top, #419ff1, #1076d5);
   -  background: url(../../../../misc/icons/000000/chevron-left.svg) left no-repeat, linear-gradient(to bottom, #419ff1, #1076d5); /* LTR */
   +  background-color: -webkit-linear-gradient(top, #419ff1, #1076d5);
   +  background-color: linear-gradient(to bottom, #419ff1, #1076d5); /* LTR */
   +  background-image: url(../../../../misc/icons/000000/chevron-left.svg); /* LTR */
   +  background-repeat: no-repeat;
   +  background-position: left;
   

   Writing these values in separate properties is less efficient then the previous implementation, we don't need to declare them separate because this is the first time we are setting these properties.

4. +++ b/core/themes/bartik/css/components/content.css
   @@ -154,36 +103,32 @@ ul.links {
    .node-preview-backlink:focus,
   -.node-preview-backlink:hover {
   +.node-preview-backlink:hover,
   +[dir="rtl"] .node-preview-backlink:focus,
   +[dir="rtl"] .node-preview-backlink:hover {
   

   We don't need to add the rtl selectors here because they are inherited anyway.

5. +++ b/core/themes/bartik/css/components/content.css
   @@ -154,36 +103,32 @@ ul.links {
   +  background-color: linear-gradient(to bottom, #59abf3, #2a90ef); /* LTR */
   

   This is not an LTR specific property so we don't need to add the comment here

6. +++ b/core/themes/bartik/css/components/content.css
   @@ -154,36 +103,32 @@ ul.links {
   +.node-preview-backlink:active,
   +[dir="rtl"] .node-preview-backlink:active {
   

   We don't need the rtl selector here

Updated patch

I think it still needs some work. There are things that change a little, like authoring font size (author and date) and the position of the body. Check screenshots:

Preview BEFORE

Preview AFTER

Node BEFORE

Node AFTER

Question to experts!

I have included node.css in bartik.libraries.yml file css/components/node.css, but to surprise, node.css was not loaded on the page when I tested it in browser. See screenshot

And that is why the CSS styles moved from content.css to node.css, is not getting applied! @Ruben has mentioned it as UI issue, which I doubt.

So two questions:

1. Why node.css is not getting loaded?
2. An alternative way to load node.css (at for testing)?

Following CSS files included in header in 'View Source'

@amolnw2778, your patch doesn't include node.css file. I can't find it in core/themes/bartik/css/components/node.css

Check patch in comment #25 and include node.css file and corrections pointed in comment #26, please.

Updated patch with node.css included

Now it's fixed. Thanks @Amol! Nice work!!

Node

Preview

1. +++ b/core/themes/bartik/bartik.libraries.yml
   @@ -15,6 +15,7 @@ global-styling:
   +      css/components/field.css: {}
   

   Where is field.css

2. +++ b/core/themes/bartik/css/components/content.css
   @@ -119,15 +69,6 @@ ul.links {
    .node-preview-container {
   

   Should this be here on in node - I would have thought node.

3. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,96 @@
   +}
   \ No newline at end of file
   

   No new line at end of file.

Trying to fix issues mentioned in #68.

Rerolled to #25 as @emma.maria said in #70. Starting to work on #26 and forward now.

Interdiff was not possible.

Fixed most of the issues from #72. Still continuing with this one.

Fixed em and page-title things from #26. Now this is really in need of review.

Looking good!

1. +++ b/core/themes/bartik/css/components/comments.css
   +++ b/core/themes/bartik/css/components/comments.css
   @@ -97,6 +97,18 @@
   
   @@ -97,6 +97,18 @@
      padding: 5px 5px 5px 2px;
    }
   +/**
   + * @todo:
   + * unpublished nodes have class .node--unpublished
   + * change this to .comment--unpublished
   + */
   

   We need a blank line before a comment

2. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,67 @@
   +.content,
   +.node__content {
   +  margin-top: 10px;
   +}
   

   Can we remove the .content class from this file and have this styling in content?

3. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,67 @@
   +.content h2 {
   +  margin-bottom: 2px;
   

   This feels like it should go inside the content file?

4. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,67 @@
   +}
   +/* View mode teaser styles*/
   +.node--view-mode-teaser .node__content {
   +  font-size: 1em;
   +}
   

   We also need a blank line before here

5. +++ b/core/themes/bartik/css/components/node.css
   @@ -0,0 +1,67 @@
   +  line-height: 1.6em;
   +}
   +/* Node metadata styles */
   +.node__meta {
   ...
   +}
   +/* Unpublished node styles */
   +.node--unpublished {
   +  padding: 20px 15px 0;
   

   Also here

Fixed #74.1-#74.5.

After applying the patch the layout is broken. Code review later.

Before:

After:

+++ b/core/themes/bartik/templates/page.html.twig
@@ -124,12 +124,12 @@
+              <h1 class="title" class="page-title">

Class is declared two times, it does not seem to work.

+++ b/core/themes/bartik/css/layout.css
@@ -178,30 +178,30 @@ body,
-  .layout-sidebar-first #content {
+  .layout-sidebar-first .content {
     margin-left: 25%; /* LTR */
     margin-right: 0; /* LTR */
   }

The goal of the layout rules defined here (and before and after) is to affect the main html element in our page template which have a content class (page.html.twig, line 127).

However some levels deeper we have a div which also has the content class (defined by block.html.twig, line 52). This exact code shifts the content to the right which can be seen on my screenshot above, but in other situations (left sidebar etc) the same class can generate further problems.

@thamas Ah yes, that is true, I think .content is too generic to be a reliable class, my suggestion is .main-content. Hopefully we will also fix the block classes soon as well to .block__content

@LevisNyman So the next patch should change the class of the main element to .main-content but changing the class of the block will be the subject of an other issue. Am I right?

It is also the .content class of div what breaks the layout of the sidebar blocks. (See screenshot in #76.)
It worth to mention that there is a .sidebar .block .content selector at line 20 in sidebar.css so we will lost its formatting when rename the block's class to block__content!

+++ b/core/themes/bartik/bartik.libraries.yml
@@ -10,18 +10,20 @@ global-styling:
-      css/components/content.css: {}

+++ b/core/themes/bartik/css/components/comments.css
@@ -97,6 +97,19 @@
diff --git a/core/themes/bartik/css/components/content.css b/core/themes/bartik/css/components/content.css

diff --git a/core/themes/bartik/css/components/content.css b/core/themes/bartik/css/components/content.css
index 6c70680..7ce5dc1 100644

index 6c70680..7ce5dc1 100644
--- a/core/themes/bartik/css/components/content.css

--- a/core/themes/bartik/css/components/content.css
+++ b/core/themes/bartik/css/components/content.css

+++ b/core/themes/bartik/css/components/content.css
+++ b/core/themes/bartik/css/components/content.css
@@ -1,193 +1,19 @@

@@ -1,193 +1,19 @@
-/* ----------------- Content ------------------ */
+/**
+ * @file
+ * Visual styles for Bartik's content component.
+ */
 
-.content,
-.node__content {
+.content {
   margin-top: 10px;

content.css is removed while it has styling??

Fixed the issues mentioned in previous comments. Also set back the main content anchor class to id whicgh had been changed by accident.

(We still have several id-s in page.html.twig. Is this the subject of an other issue?)

(I missed the patch number, sorry.)

We have different clean up issues for every component. So yes, we will get rid of all ID's in page.html.twig, but it's going to take some time.

@LevisNyman So the next patch should change the class of the main element to .main-content but changing the class of the block will be the subject of an other issue. Am I right?

Yep that makes sense, let's try and fix everything in content and avoid changing things in other files that we don't have to to fix content

Rerolled

We are almost there!

1. +++ b/core/themes/bartik/css/components/content.css
   @@ -1,188 +1,19 @@
   -h1#page-title {
   +.page-title {
      font-size: 2em;
   -  line-height: 1;
   

   Can we move the page-title component into its own file?

2. +++ b/core/themes/bartik/css/components/content.css
   @@ -1,188 +1,19 @@
    .region-content ul,
    .region-content ol {
   

   Is it possible to use the .content class here as a wrapper instead of region-content? Then all the classes would match the file name

3. +++ b/core/themes/bartik/templates/page.html.twig
   @@ -120,12 +120,12 @@
   -        <main id="content" class="column" role="main">
   +        <main class="main-content column" role="main">
   

   We've changed the class from content to main-content now right? There are still a lot of instances of the .content class, also the name of the files needs to be main-content.css instead of content.css

#87 I worked on the 3rd step of that. I am not able to understand 1st and 2nd point. Please elaborate that.

#87 I worked on the 3rd step of that. I am not able to understand 1st and 2nd point. Please elaborate that.

@Maninders Looks like your patch is missing a lot of changes from the previous patch?

@LewisNyman So I will continue from #86 and #87

Rerolled. Currently applies to the latest HEAD.

Still needs to work in things mentioned in #87

Made the changes mentioned in #87.

Also removed duplicated code from (main-)content.css which was added in the last reroll and moved new code which was added since the #87 into their own components.

The only code improvement I could find was:

+++ b/core/themes/bartik/css/components/block.css
@@ -3,6 +3,12 @@
+.content {
+  margin-top: 10px;
+}

If this is only for the block component then we should add the .block class to the selector so it can't affect anything else

Made the recommended change from #95.

Also looked for selectors which was not changed from .content to .main-content. Found some and also fixed some code comment issues of those files.

Cleaning up content.css now seems to be ready (as we are left with only some lines) but I think we need follow up issues to clean up / refactor a lot of other (new) components.

1. +++ b/core/themes/bartik/css/components/form.css
   @@ -237,9 +241,6 @@ input.form-submit:focus {
   -#content h2.comment-form {
   -  margin-bottom: 0.5em;
   -}
   

   .comment-form class does not exist anymore. Comment form's title is styled in comments.css.

2. +++ b/core/themes/bartik/css/maintenance-page.css
   @@ -24,10 +24,10 @@ body.maintenance-page {
   -.maintenance-page #content .section {
   +.maintenance-page .content .section {
      padding: 0 0 0 10px; /* LTR */
    }
   -[dir="rtl"] .maintenance-page #content .section {
   +[dir="rtl"] .maintenance-page .content .section {
      padding-left: 0;
      padding-right: 10px;
    }
   

   These rules does not apply but it is good, because it would be a broken layout. I do not know the motivation of these rules but it seems to me that they could be removed.

@thamas Ok thanks, I think a screenshot of the maintenance page would be useful to show we haven't broken anything?

Also tagging for visual regression testing.

+++ b/core/themes/bartik/css/components/admin.css
@@ -1,12 +1,16 @@
-/* ---------- Admin-specific Theming ---------- */
-.path-admin .content img {
+/**
+ * @file
+ * Visual styles for using Bartik as an adminstration theme.
+ */

+++ b/core/themes/bartik/css/components/form.css
@@ -1,11 +1,14 @@
-/* -------------- Password Field  ------------- */
+/**
+ * @file
+ * Visual styles for Bartik's forms.
+ */
...
-/* -------------- Form Elements   ------------- */
-
+/* Form elements. */

We should avoid making changes to files outside the scope of this issue so we don't overlap with other issues, can you remove these comment changes please?

Setting to needs work toe revert the comment changes about. Feels like it's a novice issue

Finished all the changes from the comment #100

Maintenance page looks the same as before the patch.

@trboslav I think there is a another patch mixed in with the patch you posted?

@trboslav thanks for your work! Something must have gone wrong because there is a lot of "noise" (unneeded code at the beginning of the patch). We can fix it in the next step if you have more energy to work on it… :)

For anyone who feels that the screenshots shows that the two states are different: they are not, only the size of the two picture differs.

+++ b/core/themes/bartik/css/components/form.css
@@ -1,11 +1,9 @@
 /* -------------- Password Field  ------------- */
-
 .password-field {
...
 /* -------------- Form Elements   ------------- */
-
 form {

As we keep the original comment style of the forms.css file (in this issue) these empty lines should not be removed I think.

Rerolled : I took #96 and applied what is suggested at #100 and #106

Maintenance page after patch :

Let's test it!

@pguillard thanks! Patch applies flawlessly.

+++ b/core/themes/bartik/css/components/form.css
@@ -1,11 +1,14 @@
-/* -------------- Password Field  ------------- */
...
-/* -------------- Form Elements   ------------- */
-

Lewis asked in #100 to keep this original comments. I do not know if it is really important.

What do you think Lewis?

So it is (almost) OK. How should it be reviewed (beyond checking the code)? Summary talks about screenshots but we could say what pages / content should be in screenshots.

Should / can we list the tasks of reviewing (by editing the summary)?

I checked the patch in #107 with siteeffect and I couldn't find any regressions.

Cool. Maybe RTBC ?

Great, thanks! Looks like it's ready to go. Good work everyone

@emma.maria Could you share the backtopjs.json you used for this test? Thanks!

Thanks @emma.maria! I create a similar test and will fix the problems.

@thamas, are you still working on this issue? I got asked to look at this issue at the devdays as well, and i also have a solution. Thnx!

So biggest problem was that node.css wasn't loading. After a clearing my cache, it did load and there were only a few minor visual differences, which i fixed.

@haasontwerp @emma.maria Sorry for being unavailable, I was on my way to home in the last days.

Patch does not apply to the latest head… :(

Hope I rerolled it rightly.

Patch applies to the latest head but there are visual regressions. (See the attached pdf.)

New patch. Applies to the latest head. No visual regressions. Comments later. :)

Great work! I found only a few things to fix.

+++ b/core/themes/bartik/css/components/comments.css
@@ -116,6 +116,19 @@
+ * @todo:
+ * unpublished nodes have class .node--unpublished
+ * change this to .comment--unpublished

The @todo comments don't usually break the line, also this comment should have a full stop at the end

1. +++ b/core/themes/bartik/bartik.libraries.yml
   @@ -10,19 +10,25 @@ global-styling:
   +      css/components/footer.css: {}
   

   This is adding a footer.css file that doesn't exist, the file is called site-footer.css now, so we can just delete this line

2. +++ b/core/themes/bartik/css/components/ui.widget.css
   index bb23434..0112af4 100644
   --- a/core/themes/bartik/css/layout.css
   
   --- a/core/themes/bartik/css/layout.css
   +++ b/core/themes/bartik/css/layout.css
   
   +++ b/core/themes/bartik/css/layout.css
   +++ b/core/themes/bartik/css/layout.css
   @@ -11,10 +11,208 @@
   
   @@ -11,10 +11,208 @@
      margin-left: auto;
      margin-right: auto;
    }
   +#header div.section {
   +  position: relative;
   +}
   

   This patch is adding all the CSS back into the layout.css file that was removed in #2398451: Clean up "layout" CSS in Bartik. I think we should be able to remove all of this CSS, let's just make sure that the main-content selectors are replicated in main-content.css

Changes as suggested in #131

Also @lewis

#header div.section {
  position: relative;
}

is already in header.css file. you can find it as

.header .section {
  position: relative;
}

Check the patch.

1. +++ b/core/themes/bartik/css/components/comments.css
   @@ -116,6 +116,17 @@
   +/**
   + * @todo: unpublished nodes have class .node--unpublished change this to .comment--unpublished.
   

   When the length of this line goes over 80 characters, then we have to break the line. We should break to a new line before ".comment--unpublished"

2. +++ b/core/themes/bartik/css/components/ui.widget.css
   index bb23434..0112af4 100644
   --- a/core/themes/bartik/css/layout.css
   
   --- a/core/themes/bartik/css/layout.css
   +++ b/core/themes/bartik/css/layout.css
   

   We are still adding loads of CSS into layout.css that we removed before, we need to remove all these lines.

Split long (80+ char) comment line in comments.css file into two lines.

Modified layout.css file to eliminate class selectors that have previously been moved to other css files.

Changing status to "needs review"

Great, thank you!

re-rtbc

https://www.drupal.org/patch/reroll

patch 146 breaks layout -- no sidebars,etc . Patch 132 looks a good candidate to work forward from.

emma.maria: interdiffs are not possible for rerolls :)

@emma no issues.

ignore me - accidentally pressed submit on the wrong tab!!!

(hides in corner for a bit)

It looks like there was some styling that was not moved from content.css into another CSS file. It would be worth someone going through each line that was removed to make sure that it was moved somewhere else.

Wanted to take a look at this but first I seem to need to re-roll again...

Trigger testbot

@emma.maria i am not able to reproduce issue which you shown via screenshots in #156. Can you please try to clear a cache.
when i cleared my cache its something looks like this. please check below screenshot

@prabhurajn654 - can you post a screenshot of what that same content looks like running on current 8.0.x branch, please? Would be good to see you comparison.

Also, which browser(s)?

Thanks

Ha ha - after this week, I'm going to change my d.o username to re-roll-rachel!

@emma.maria we are trying to keep with you! :)

Committed d5669ee and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.