Beta phase evaluation
| 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?
| Comment | File | Size | Author |
|---|---|---|---|
| #85 | 446 bytes | sqndr | |
| #85 | 2.56 KB | sqndr | |
| PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,065 pass(es). [ View ] | |||
| #58 | 3.61 KB | Outi | |
| 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 | 1.89 KB | Outi | |
| #54 | 3.43 KB | Outi | |
| PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,998 pass(es). [ View ] | |||
Comments
Comment #1
kid_icarus commentedComment #2
LewisNyman commentedI 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.
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"
Comment #3
LewisNyman commentedtags
Comment #4
frankbaele commentedok 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.
Comment #6
frankbaele commented#4: drupal-print_stylesheet-2.patch queued for re-testing.
Comment #8
frankbaele commentedcurrently blocked by #2031317: Color upgrade path test is broken
Comment #9
tompagabor commented4: drupal-print_stylesheet-2.patch queued for re-testing.
Comment #11
tompagabor commentedReroll for simpletest.
Comment #13
tompagabor commentedOK, 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?
Comment #14
tompagabor commentedHere is the new version. It"s use many things from hml5boilerplate css:
https://github.com/h5bp/html5-boilerplate/blob/master/css/main.css
Comment #16
Outi commentedI 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.
Comment #17
tompagabor commentedThanks 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.
Comment #18
visabhishek commentedI am changing the status to review.
Comment #20
tompagabor commentedAdd files, with do-not-test, see #13.
After we finish the print layout, then make a patch for the test.
Comment #21
mathieso commentedToolbar 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?
Comment #22
LewisNyman commentedYeah we should
Comment #23
tompagabor commentedHere is the new one.
Comment #24
LewisNyman commentedComment #25
jlyon commentedRe-rolled this patch for the latest d8-dev and added a declaration to hide the toolbar in print versions.
Comment #26
eporama commentedSince 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.
Comment #29
tompagabor commentedReroll.
Comment #30
aschiwi commentedTesting
Comment #31
aschiwi commentedPatch can't be applied using "git apply". This is the message shown:
git apply --index 1892006-print_style-29.patcherror: 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.
Comment #32
criscom commentedRerolled and applied patch successfully with no errors. Uploaded patch.
Comment #33
criscom commentedComment #34
LewisNyman commented@criscom The patch needs review from one more person before we can RTBC
Comment #35
leonrenkema commentedReviewing
Comment #38
leonrenkema commentedReviewed and it worked. Tested in Safari, Firefox and Chrome on Mac
Comment #39
herom commented+++ 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.
Comment #40
skippednote commentedAdding #32 patch without "layout-column" related theming.
Comment #41
leonrenkema commentedYes, looks better. Okay with me.
Comment #42
sqndr commentedI'm going to update the issue summary.
Comment #43
sqndr commentedUpdated the summary. Also been hiding some of the files. This issues still needs a review, manual testing and screenshots.
Comment #44
skippednote commentedThe buttons size, spacing and color contrast needs to be fixed.
Comment #45
Outi commentedI 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.
Comment #46
tompagabor commentedUpdated patch, made screenshots about the fixes, remove duplicated format for #toolbar-administration.
Comment #49
Outi commentedIt looks good for me with the patch #46.
Comment #50
Outi commentedComment #51
LewisNyman commentedThanks, it would be good if we could move this into it's own print.css file for now, layout does not make sense.
Comment #52
Outi commentedI agree that layout.css doesn't seem to be the right place, but the description says
If you prefer it to be in a separate file, I'm ok with that.
Comment #53
Outi commentedI create in this patch a separated print.css file. I'm not sure if I've done everything in the right way:
Comment #54
Outi commentedI added the new line in the end of print.css, fixed the .info file and added the print.css to the ThemeHandlerTest.php.
Comment #55
Outi commentedComment #58
Outi commentedThis patch fixes the style sheet declaration order in the .info file.
Comment #59
BarisW commentedWow, that's a nice improvement! Here's two screenshots of #85, one without and one with the patch applied.
Before:
After:

Looks good to me!
Comment #60
LewisNyman commentedLooking good to me. Thanks
Comment #61
sqndr commentedOtherwise, we could always include:
@media only print{
body * { display: none !important; }
body:after { content: "Don't waste paper!"; }
}
-- http://printstylesheet.com/
Comment #63
herom commentedRerolled, only context lines were changed. So, back to RTBC.
Comment #64
alexpott commented+++ 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.
+++ 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?
+++ 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?
Comment #65
LewisNyman commentedDropbuttons 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.
hmm, we don't set appearance in Seven's form.css. Is anyone able to explain the logic behind this?
-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.Comment #66
vermario commentedRerolled the patch, removing the webkit only directive as suggested in #65.
Comment #67
sqndr commented+++ b/core/themes/seven/css/base/print.css@@ -0,0 +1,80 @@
+ -webkit-appearance: none;
See #65.
Comment #68
vermario commentedRight! :) Removed the additional webkit only rule.
Comment #69
LewisNyman commentedLooks like Alex's issues have been address, here's an interdiff from 63-68 to confirm.
Comment #71
BarisW commentedHere's a re-roll.
Comment #72
BarisW commentedHmm 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',
?>
Comment #73
LewisNyman commentedYep looks like DRUPAL_ROOT has been changed to this->root. That's the only difference
Comment #74
alexpott commentedTo 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.
Comment #75
vermario commentedI have added the requested template. How do we make a decision on this now?
Comment #76
LewisNyman commentedNow that we have the template I think we can move it back to RTBC.
Comment #78
LewisNyman commentedComment #79
lauriii commentedReroll after: #2377397: Themes should use libraries, not individual stylesheets
Comment #81
herom commentedFixing the reroll.
Comment #82
LewisNyman commentedA quick manual test and all is well.
Comment #83
alexpott commented+++ 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
Comment #84
sqndr commentedFixed the issue from #83.
Update: Something went wrong. Check the next comment for the correct patch.
Comment #85
sqndr commentedSomething went wrong. Here's the patch and the interdiff.
Comment #86
LewisNyman commented@alexpott Nice catch :-) Yep this is what we want. It reduces the number of loaded CSS files by one.
Thanks Sander for the patch.
Comment #87
alexpott commentedCommitted 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.