Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Not critical , described as an edge case

It's an egde case, but it might be possible that people what to print an admin page. To accomplish this, we would use a media query (instead of a separate file, following the best practice from HTML5 Boilerplate). The main stylesheet should then include in the media all, instead of screen. Since this last changes breaks the test, the test should be updated as well (#26).

Remaining tasks:
- Review the last submitted patch. (needs review)
- Add screenshots of the print styling. (needs screenshots)
- RTBC.

Original message:

I guess people still print web pages. So, it seems reasonable to include a print stylesheet in the Seven theme. This patch introduces a print stylesheet, with one very basic rule that simply hides the administrative toolbar from the printed page.

Thoughts?

Files: 
CommentFileSizeAuthor
#85 interdiff.txt446 bytessqndr
#85 include_a_print_styling-1892006-85.patch2.56 KBsqndr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,065 pass(es).
[ View ]
#58 1892006-print_style-57.patch3.61 KBOuti
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1892006-print_style-57.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#54 interdiff.txt1.89 KBOuti
#54 1892006-print_style-54.patch3.43 KBOuti
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,998 pass(es).
[ View ]
#53 1892006-print_style-53.patch3.46 KBOuti
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,877 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#49 print-seven-pathc-46-2.png72.69 KBOuti
#49 print-seven-patch-46-1.png101.49 KBOuti
#46 1892006-print_style-46.patch3.27 KBtompagabor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,870 pass(es).
[ View ]
#45 print-seven-patch-40-2.png87.59 KBOuti
#45 print-seven-patch-40-1.png131.38 KBOuti
#44 2.png24.72 KBskippednote
#44 1.png55.21 KBskippednote
#40 seven_needs_a_print-stylesheet-1892006-40.patch3.1 KBskippednote
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,761 pass(es).
[ View ]
#23 1892006-print_style-23-do-not-test.patch2.26 KBtompagabor
#20 1892006-print_style-17-do-not-test.patch2.24 KBtompagabor
#17 1892006-print_style-17.patch2.24 KBtompagabor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,850 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 print-seven-patch-14-1.png46.32 KBOuti
#16 print-seven-patch-14-2.png50.06 KBOuti
#16 print-seven-patch-14-3.png70.55 KBOuti
#14 1892006-print_style-14.patch1.97 KBtompagabor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,785 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 1892006-reroll.patch866 bytestompagabor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,739 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 drupal-print_stylesheet-2.patch886 bytesfrankbaele
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-print_stylesheet-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 Screen Shot 2013-06-22 at 10.18.08.png37.41 KBLewisNyman
drupal-print_stylesheet-1.patch758 byteskid_icarus
PASSED: [[SimpleTest]]: [MySQL] 50,563 pass(es).
[ View ]

Comments

kid_icarus’s picture

Status:Active» Needs review
LewisNyman’s picture

Status:Needs review» Needs work
StatusFileSize
new37.41 KB

I think it's entirely possible that someone may want to print an admin page but it's an extreme edge case. I'm not sure if it's worth the extra weight in CSS, the time, or the paper to get this looking good.

However, given this is how a printed page currently looks in Seven, I think this one line fix to hide the toolbar is a big gain at little cost.

Screen Shot 2013-06-22 at 10.18.08.png

I'd rather follow best practices here and include the print CSS in side of a media query instead of in a separate file, the browser downloads both on page load anyway. We're going to have to set the main stylesheet to media "all" instead of "screen"

LewisNyman’s picture

Issue tags:+CSS, +Novice, +css-novice

tags

frankbaele’s picture

Status:Needs work» Needs review
StatusFileSize
new886 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-print_stylesheet-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok moved the print styling in to a media query and changed the style media from screen to all, to make the print media query work.

Status:Needs review» Needs work
Issue tags:-CSS, -Novice, -css-novice

The last submitted patch, drupal-print_stylesheet-2.patch, failed testing.

frankbaele’s picture

Status:Needs work» Needs review

#4: drupal-print_stylesheet-2.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+CSS, +Novice, +css-novice

The last submitted patch, drupal-print_stylesheet-2.patch, failed testing.

frankbaele’s picture

tompagabor’s picture

Status:Needs work» Needs review

4: drupal-print_stylesheet-2.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 4: drupal-print_stylesheet-2.patch, failed testing.

tompagabor’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new866 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,739 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Reroll for simpletest.

Status:Needs review» Needs work

The last submitted patch, 11: 1892006-reroll.patch, failed testing.

tompagabor’s picture

OK, we have a new problem with the patch.
The testRebuildThemeData function throw the error. the test always want the 'screen' array instead of 'all'.

core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php, line 281.

    $this->assertEquals(array(
      'screen' => array(
        'seven.base.css' => DRUPAL_ROOT . '/core/themes/seven/seven.base.css',

Could we change this test?

tompagabor’s picture

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,785 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here is the new version. It"s use many things from hml5boilerplate css:
https://github.com/h5bp/html5-boilerplate/blob/master/css/main.css

Status:Needs review» Needs work

The last submitted patch, 14: 1892006-print_style-14.patch, failed testing.

Outi’s picture

StatusFileSize
new70.55 KB
new50.06 KB
new46.32 KB

I tested the patch #14 (after removing the whitespace on the line 88), and it seems to have some problems with the select lists that causes vertical lines. I haven't printed the pages but only taken screenshots of the preview before printing, so I'm not sure if the lines would really be printed or if it's just the preview.

I'm not sure if printing the path of each link is useful or somewhat confusing instead.

tompagabor’s picture

StatusFileSize
new2.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,850 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Thanks for testing.

I removed whitespace, and change 4space tabs to 2spaces.
Add fix for select box, and another form elements, to not show vertical lines across the print layout.
Tested more times the link with/without the href, and i removed it.
I tested print layout to print PDF files.
Added support to show dropbutton links.
Some fix on tabs navigation.
Added some code, like show background-images on dblog on chrome.

visabhishek’s picture

Status:Needs work» Needs review

I am changing the status to review.

Status:Needs review» Needs work

The last submitted patch, 17: 1892006-print_style-17.patch, failed testing.

tompagabor’s picture

Status:Needs work» Needs review
StatusFileSize
new2.24 KB

Add files, with do-not-test, see #13.
After we finish the print layout, then make a patch for the test.

mathieso’s picture

Toolbar shows in print preview on FireFox/Ubuntu. It repeats at the top of every page in the preview. So if the prviews has page 1 of 4, 2 of 4, etc., the toolbar will be on every page.

Same effect whether the second line of the toolbar is showing or not. Not affected by which page is being previewed.

Remove the toolbar entirely from the print view?

LewisNyman’s picture

Yeah we should

tompagabor’s picture

StatusFileSize
new2.26 KB

Here is the new one.

LewisNyman’s picture

Issue tags:+frontend
jlyon’s picture

StatusFileSize
new2.26 KB

Re-rolled this patch for the latest d8-dev and added a declaration to hide the toolbar in print versions.

eporama’s picture

StatusFileSize
new3.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1892006-print_style-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Since you are changing the .info.yml file to be reading the media as "all" then we should go ahead and straight forwardly change the test to match.

Here is the same patch rerolled with the test included, so renaming the patch so it will kick off a test run.

Status:Needs review» Needs work

The last submitted patch, 26: 1892006-print_style-26.patch, failed testing.

tompagabor’s picture

Status:Needs work» Needs review
StatusFileSize
new3.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,795 pass(es).
[ View ]

Reroll.

aschiwi’s picture

Testing

aschiwi’s picture

Status:Needs review» Needs work

Patch can't be applied using "git apply". This is the message shown:

git apply --index 1892006-print_style-29.patch
error: patch failed: core/themes/seven/css/layout/layout.css:42
error: core/themes/seven/css/layout/layout.css: patch does not apply

When applying it with the patch command, a file called layout.css.orig gets created.

criscom’s picture

Status:Needs work» Needs review
Issue tags:+Amsterdam2014
StatusFileSize
new3.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,763 pass(es).
[ View ]

Rerolled and applied patch successfully with no errors. Uploaded patch.

criscom’s picture

Status:Needs review» Reviewed & tested by the community
LewisNyman’s picture

Status:Reviewed & tested by the community» Needs review

@criscom The patch needs review from one more person before we can RTBC

leonrenkema’s picture

Reviewing

The last submitted patch, 26: 1892006-print_style-26.patch, failed testing.

leonrenkema’s picture

Status:Needs review» Reviewed & tested by the community

Reviewed and it worked. Tested in Safari, Firefox and Chrome on Mac

herom’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/themes/seven/css/layout/layout.css
@@ -18,10 +18,10 @@
-  .layout-column + .layout-column {
+  .layout-column  .layout-column {
...
-  [dir="rtl"] .layout-column + .layout-column {
+  [dir="rtl"] .layout-column  .layout-column {

I think these got in accidently in the #29 reroll.

skippednote’s picture

StatusFileSize
new3.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,761 pass(es).
[ View ]

Adding #32 patch without "layout-column" related theming.

leonrenkema’s picture

Status:Needs work» Needs review

Yes, looks better. Okay with me.

sqndr’s picture

I'm going to update the issue summary.

sqndr’s picture

Title:Seven needs a print stylesheet.Include a print styling for Seven.
Issue summary:View changes
Issue tags:+needs screenshots

Updated the summary. Also been hiding some of the files. This issues still needs a review, manual testing and screenshots.

skippednote’s picture

Status:Needs review» Needs work
StatusFileSize
new55.21 KB
new24.72 KB

The buttons size, spacing and color contrast needs to be fixed.

Outi’s picture

StatusFileSize
new131.38 KB
new87.59 KB

I tested with the patch #40, and otherwise it seems ok but the operation options transformed in circles should be fixed. The first screenshot is from admin/structure/views, the second one from admin/structure/types/manage/article/fields.

On the views page, the button that is blue on the screen is white on my printed sheet (so there is no contrast problem), but on the field management page it is blue also on the printed sheet.

I tested with Firefox.

tompagabor’s picture

Status:Needs work» Needs review
StatusFileSize
new3.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,870 pass(es).
[ View ]
new18.06 KB
new25.36 KB

Updated patch, made screenshots about the fixes, remove duplicated format for #toolbar-administration.

Status:Needs review» Needs work

The last submitted patch, 46: 1892006-print_style-46.patch, failed testing.

Outi’s picture

StatusFileSize
new101.49 KB
new72.69 KB

It looks good for me with the patch #46.

Outi’s picture

Status:Needs work» Needs review
LewisNyman’s picture

Status:Needs review» Needs work

Thanks, it would be good if we could move this into it's own print.css file for now, layout does not make sense.

Outi’s picture

I agree that layout.css doesn't seem to be the right place, but the description says

To accomplish this, we would use a media query (instead of a separate file, following the best practice from HTML5 Boilerplate).

If you prefer it to be in a separate file, I'm ok with that.

Outi’s picture

StatusFileSize
new3.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,877 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I create in this patch a separated print.css file. I'm not sure if I've done everything in the right way:

  • Should the print.css file include the media query? (Here it does.)
  • Should the print.css file be listed in the .info file in a separated 'print' section? (Here it is.)
  • Should the ThemeHandlerTest.php be modified? (Here it isn't besides what was done in the previous patch.)
  • I put the print.css file in core/themes/seven/css/base/. Does it make sense?
Outi’s picture

StatusFileSize
new3.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,998 pass(es).
[ View ]
new1.89 KB

I added the new line in the end of print.css, fixed the .info file and added the print.css to the ThemeHandlerTest.php.

Outi’s picture

Status:Needs work» Needs review

The last submitted patch, 53: 1892006-print_style-53.patch, failed testing.

Outi’s picture

StatusFileSize
new3.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1892006-print_style-57.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This patch fixes the style sheet declaration order in the .info file.

BarisW’s picture

StatusFileSize
new59.11 KB
new51.7 KB

Wow, that's a nice improvement! Here's two screenshots of #85, one without and one with the patch applied.

Before:
The People page without the patch appliedAfter:
The People page with the patch applied

Looks good to me!

LewisNyman’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-needs screenshots

Looking good to me. Thanks

sqndr’s picture

Otherwise, we could always include:

@media only print
{
    body * { display: none !important; }
    body:after { content: "Don't waste paper!"; }
}

-- http://printstylesheet.com/

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 58: 1892006-print_style-57.patch, failed testing.

herom’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,409 pass(es).
[ View ]

Rerolled, only context lines were changed. So, back to RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review
  1. +++ b/core/themes/seven/css/base/print.css
    @@ -0,0 +1,83 @@
    +  .dropbutton-multiple .dropbutton .secondary-action {
    +    display: block;
    +  }
    +  .js .dropbutton-widget,
    +  .js td .dropbutton-widget /* Splitbuttons */ {
    +    position: relative;
    +  }
    +  .js .dropbutton .dropbutton-toggle {
    +    display: none;
    +  }
    +  .js .dropbutton-multiple .dropbutton-widget {
    +    background: none;
    +    border-radius: 4px;
    +  }

    More special stuff for dropbuttons - this will break before release.

  2. +++ b/core/themes/seven/css/base/print.css
    @@ -0,0 +1,83 @@
    +  input.form-autocomplete, input.form-text, input.form-tel, input.form-email, input.form-url, input.form-search, input.form-number, input.form-color, input.form-file, textarea.form-textarea, select.form-select {
    +    -webkit-appearance: none;
    +    border-width: 1px;
    +  }

    Why is their a webkit only directive here?

  3. +++ b/core/themes/seven/css/base/print.css
    @@ -0,0 +1,83 @@
    +  .admin-dblog .icon {
    +    -webkit-print-color-adjust:exact;
    +  }

    Are we sure we want to specifically target webkit browsers here?

LewisNyman’s picture

Status:Needs review» Needs work

More special stuff for dropbuttons - this will break before release.

Dropbuttons require special print styling because they have special styling. I don't see the problem here. It's a bit weird that js is running on a printed page but I guess a lot of sites these days would look broken without JS.

Why is their a webkit only directive here?

hmm, we don't set appearance in Seven's form.css. Is anyone able to explain the logic behind this?

Are we sure we want to specifically target webkit browsers here?

-webkit-print-color-adjust:exact; is a webkit only property that does not exist in other browsers. It's an explicit direction to print background images. In #2236855: Use CSS for file icons in file fields we decided that there is no need to print purely presentational icons. I think we can remove this line.

vermario’s picture

Status:Needs work» Needs review
StatusFileSize
new3.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,159 pass(es).
[ View ]

Rerolled the patch, removing the webkit only directive as suggested in #65.

sqndr’s picture

Status:Needs review» Needs work
+++ b/core/themes/seven/css/base/print.css
@@ -0,0 +1,80 @@
+    -webkit-appearance: none;

See #65.

vermario’s picture

Status:Needs work» Needs review
StatusFileSize
new3.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1892006-print_style-68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Right! :) Removed the additional webkit only rule.

LewisNyman’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Novice, -css-novice
StatusFileSize
new1.31 KB

Looks like Alex's issues have been address, here's an interdiff from 63-68 to confirm.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 68: 1892006-print_style-68.patch, failed testing.

BarisW’s picture

Status:Needs work» Needs review
StatusFileSize
new3.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1892006-print_style-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new0 bytes

Here's a re-roll.

BarisW’s picture

Hmm interdiff failed. But the only difference is from this:

<?php
     $this
->assertEquals(array(
-     
'screen' => array(
+     
'all' => array(
        
'css/base/elements.css' => DRUPAL_ROOT . '/core/themes/seven/css/base/elements.css',
        
'css/base/typography.css' => DRUPAL_ROOT . '/core/themes/seven/css/base/typography.css',
+       
'css/base/print.css' => DRUPAL_ROOT . '/core/themes/seven/css/base/print.css',
        
'css/components/admin-list.css' => DRUPAL_ROOT . '/core/themes/seven/css/components/admin-list.css',
        
'css/components/admin-options.css' => DRUPAL_ROOT . '/core/themes/seven/css/components/admin-options.css',
        
'css/components/admin-panel.css' => DRUPAL_ROOT . '/core/themes/seven/css/components/admin-panel.css',
?>

to:

<?php
     $this
->assertEquals(array(
-     
'screen' => array(
+     
'all' => array(
        
'css/base/elements.css' => $this->root . '/core/themes/seven/css/base/elements.css',
        
'css/base/typography.css' => $this->root . '/core/themes/seven/css/base/typography.css',
+       
'css/base/print.css' => $this->root . '/core/themes/seven/css/base/print.css',
        
'css/components/admin-list.css' => $this->root . '/core/themes/seven/css/components/admin-list.css',
        
'css/components/admin-options.css' => $this->root . '/core/themes/seven/css/components/admin-options.css',
        
'css/components/admin-panel.css' => $this->root . '/core/themes/seven/css/components/admin-panel.css',
?>
LewisNyman’s picture

Status:Needs review» Reviewed & tested by the community

Yep looks like DRUPAL_ROOT has been changed to this->root. That's the only difference

alexpott’s picture

Category:Feature request» Task
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs issue summary update

To be honest this does not really feel like a feature - printing web pages is something people can do regardless of this patch so redefining as a task. Since this issue is now a normal task we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

vermario’s picture

Issue summary:View changes

I have added the requested template. How do we make a decision on this now?

LewisNyman’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs issue summary update

Now that we have the template I think we can move it back to RTBC.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 71: 1892006-print_style-71.patch, failed testing.

LewisNyman’s picture

Issue tags:+Needs reroll
lauriii’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new2.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,906 pass(es), 0 fail(s), and 140 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 79: include_a_print_styling-1892006-79.patch, failed testing.

herom’s picture

Status:Needs work» Needs review
StatusFileSize
new675 bytes
new2.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,995 pass(es).
[ View ]

Fixing the reroll.

LewisNyman’s picture

Status:Needs review» Reviewed & tested by the community

A quick manual test and all is well.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/themes/seven/css/base/print.css
@@ -0,0 +1,79 @@
+@media print {

+++ b/core/themes/seven/seven.libraries.yml
@@ -4,6 +4,7 @@ global-styling:
+      css/base/print.css: { media: print }

What is the point of having the media query in both places? According to the issue summary boilerplate suggests using a media query instead of a separate file. If we want this to be aggregated into Seven's global-styling css then I think we need to remove the media query from seven.libraries.yml

sqndr’s picture

Status:Needs work» Needs review
StatusFileSize
new685 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,080 pass(es).
[ View ]
new2.2 KB

Fixed the issue from #83.

Update: Something went wrong. Check the next comment for the correct patch.

sqndr’s picture

StatusFileSize
new2.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,065 pass(es).
[ View ]
new446 bytes

Something went wrong. Here's the patch and the interdiff.

LewisNyman’s picture

Status:Needs review» Reviewed & tested by the community

@alexpott Nice catch :-) Yep this is what we want. It reduces the number of loaded CSS files by one.

Thanks Sander for the patch.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 7aef0c7 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

+++ b/core/themes/seven/seven.libraries.yml
@@ -4,6 +4,7 @@ global-styling:
diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml

diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php

diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
old mode 100644
new mode 100755

We should be changing the modes of these files - fixed on commit.

  • alexpott committed 7aef0c7 on 8.0.x
    Issue #1892006 by Outi, tompagabor, sqndr, BarisW, herom, skippednote,...