Clean up the "print.css" code in Bartik

Hi emma.maria, yes i'm working on it. If i get blocked by something ill ping you. Thanks for asking.

Changes in this patch are adding class in markup and replacing id's with class.

Adding file comment :)

+++ b/core/themes/bartik/css/print.css
@@ -33,11 +35,15 @@ body {
+#featured-bottom-first,
+#featured-bottom-second,
+#featured-bottom-third {

@simply021 I think this also need to be replaced with classes.

Rerolled. Something went awry. Should be ok.

Hi, I'm working on a patch for this issue.

Before I go ahead and submit a patch, I'd like to ask about a couple of things.

1) D7 Bartik print styles hide the comment area title "Comments". I believe this is not intentional. The style was probably meant to hide just the comment form title "Add new comment" but it accidentally hides the "Comments" title too (since they are both h2.title). Should we leave the "Comments" title visible in D8 Bartik? I attached some screenshots.

2) How important is IE8 support in D8 Bartik? I noticed "Toolbar" module is using a media query to hide the admin toolbar for print. This of course works on all modern browsers but does not do the job in IE8. Should we keep something like ".toolbar { display: none; }" in the Bartik print css to support IE8 admins who want to print some pages?

3) There's a selector weight issue regarding this style: ".comment-forbidden { display: none; }". We want to hide this node-link but it's getting a "display:inline" from another selector with a greater weight so our "display:none" has no effect. Normally I would never use an !important in a project, but this might be case where it's a viable choice. These are the two options:

• .comment-forbidden { display: none !important; }
• ul.inline li.comment-forbidden { display: none; }

Should we use the first option?

I decided to do the following in this patch:

1) Keep the "Comments" heading visible in print styles.

2) Remove "Toolbar" related print style from Bartik. I talked with @lauriii about IE8 and apparently there's no reason to support IE8.

3) Selector weight issue regarding ".comment-forbidden" is solved. (The attached screenshot clarifies what that element is.)

Maybe we should follow Seven theme's example and use a media query for the print styles? This patch does that. Here's a link to the issue about Seven's print styles: https://www.drupal.org/node/1892006.

If we want to go a step further, we could move print styles to each corresponding component's css file, wrapped in a print media query of course. For example comment print styles would go to components/comments.css and featured-bottom region styles would go to components/featured-bottom etc. There's some pros and cons in this approach but I think it might be a good idea. Any thoughts?

Three things happening in this patch:

A) Body width has been removed. I did some research and came to the conclusion that there is no need to set a body width for printing.

B) Sidebar related styles have been removed (main-content width 100%). In D7 Bartik this declaration was used to reset styles from layout.css but in D8 Bartik we don't need to do this anymore since the layout styles are in a media query and won't affect print styles.

C) Featured bottom area styles have been rewritten. This is now a copy-paste from components/featured-bottom.css (I'll make a new issue about combining print styles to the component styles). Regarding this area, I think we have three options: 1) We could show the regions one below the other (mobile style), or 2) Side by side (desktop style), or 3) We don't show this area at all. I chose the second option because on a paper we have enough space to show them side by side. And also because if one of the regions happens to have an image, it would be printed out in huge size (see attached print preview pdf files). Hiding this area in print might be a good idea as well.

1. +++ b/core/themes/bartik/css/components/main-content.css
   @@ -12,7 +12,7 @@
   -@media all and (min-width: 851px) {
   +@media screen and (min-width: 851px) {
   

   Sidebars won't be visible on print so we don't want to make main-content narrow either.

2. +++ b/core/themes/bartik/css/print.css
   @@ -4,44 +4,58 @@
      /**
   ...
   +   * Regions
   +   *
   +   * Hide regions which are unlikely to have relevant content for printing.
       */
   ...
   +  .highlighted,
   +  .featured-top,
   +  .sidebar,
   +  .featured-bottom,
   +  .site-footer {
   +    display: none;
      }
   

   I had a chat with @mortendk and we agreed that most regions should be hidden on print layout. I can elaborate more if needed.

3. +++ b/core/themes/bartik/css/print.css
   @@ -4,44 +4,58 @@
   +  /**
   +   * Navigation elements.
   +   */
   +  nav:not(.breadcrumb) {
   +    display: none;
      }
   

   Almost all nav elements are irrelevant on print.

...

+++ b/core/themes/bartik/templates/page.html.twig
@@ -101,14 +101,14 @@
+          <div id="sidebar-first" class="sidebar-first column sidebar ">

Why are the sidebar-first and sidebar-second classes being added here? They don't seemed to be referenced in any rule being changed. Also, there is a trailing space before the close quote. That should be removed.

Based on #22 removing unused classes sidebar-first and sidebar-second so as unnecessary space at the end of closing tag.

updating status to trigger tests

I have applied #22 patch and it works fine. Code changes are reflected but I can't see any UI changes, when print screen is used. Please find the attached screen shots (PDF).

@bandanasharma I am changing its status as its issue summary needs to be update.

patch #22 failed to apply on 9.1.x.
patch Re-rolled to the 9.1.x branch

Patch #38 tested and applied successfully on Drupal 9.5.x.

Patch #38 tested and applied successfully on Drupal 9.5.x.

Bartik has been moved to contrib. Please update the patches against the contributed project.

Updated patch #38 by addressing #46, please review it.

Please put the patch into an issue fork and merge request.