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 | interdiff.txt | 446 bytes | sqndr |
#85 | include_a_print_styling-1892006-85.patch | 2.56 KB | sqndr |
#58 | 1892006-print_style-57.patch | 3.61 KB | Outi |
#54 | interdiff.txt | 1.89 KB | Outi |
#54 | 1892006-print_style-54.patch | 3.43 KB | Outi |
Comments
Comment #1
kid_icarus CreditAttribution: kid_icarus commentedComment #2
LewisNymanI 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
LewisNymantags
Comment #4
frankbaele CreditAttribution: 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 CreditAttribution: frankbaele commented#4: drupal-print_stylesheet-2.patch queued for re-testing.
Comment #8
frankbaele CreditAttribution: frankbaele commentedcurrently blocked by #2031317: Color upgrade path test is broken
Comment #9
tompagabor CreditAttribution: tompagabor commented4: drupal-print_stylesheet-2.patch queued for re-testing.
Comment #11
tompagabor CreditAttribution: tompagabor commentedReroll for simpletest.
Comment #13
tompagabor CreditAttribution: 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.
Could we change this test?
Comment #14
tompagabor CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: visabhishek commentedI am changing the status to review.
Comment #20
tompagabor CreditAttribution: 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 CreditAttribution: 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
LewisNymanYeah we should
Comment #23
tompagabor CreditAttribution: tompagabor commentedHere is the new one.
Comment #24
LewisNymanComment #25
jlyon CreditAttribution: jlyon commentedRe-rolled this patch for the latest d8-dev and added a declaration to hide the toolbar in print versions.
Comment #26
eporama CreditAttribution: 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 CreditAttribution: tompagabor commentedReroll.
Comment #30
aschiwi CreditAttribution: aschiwi commentedTesting
Comment #31
aschiwi CreditAttribution: aschiwi commentedPatch can't be applied using "git apply". This is the message shown:
When applying it with the patch command, a file called layout.css.orig gets created.
Comment #32
criscomRerolled and applied patch successfully with no errors. Uploaded patch.
Comment #33
criscomComment #34
LewisNyman@criscom The patch needs review from one more person before we can RTBC
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedReviewing
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedReviewed and it worked. Tested in Safari, Firefox and Chrome on Mac
Comment #39
herom CreditAttribution: herom commentedI think these got in accidently in the #29 reroll.
Comment #40
skippednote CreditAttribution: skippednote commentedAdding #32 patch without "layout-column" related theming.
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, looks better. Okay with me.
Comment #42
sqndr CreditAttribution: sqndr commentedI'm going to update the issue summary.
Comment #43
sqndr CreditAttribution: sqndr commentedUpdated the summary. Also been hiding some of the files. This issues still needs a review, manual testing and screenshots.
Comment #44
skippednote CreditAttribution: skippednote commentedThe buttons size, spacing and color contrast needs to be fixed.
Comment #45
Outi CreditAttribution: 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 CreditAttribution: tompagabor commentedUpdated patch, made screenshots about the fixes, remove duplicated format for #toolbar-administration.
Comment #49
Outi CreditAttribution: Outi commentedIt looks good for me with the patch #46.
Comment #50
Outi CreditAttribution: Outi commentedComment #51
LewisNymanThanks, 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 CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: Outi commentedComment #58
Outi CreditAttribution: Outi commentedThis patch fixes the style sheet declaration order in the .info file.
Comment #59
BarisW CreditAttribution: 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
LewisNymanLooking good to me. Thanks
Comment #61
sqndr CreditAttribution: sqndr commentedOtherwise, we could always include:
-- http://printstylesheet.com/
Comment #63
herom CreditAttribution: herom commentedRerolled, only context lines were changed. So, back to RTBC.
Comment #64
alexpottMore special stuff for dropbuttons - this will break before release.
Why is their a webkit only directive here?
Are we sure we want to specifically target webkit browsers here?
Comment #65
LewisNymanDropbuttons 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 CreditAttribution: vermario commentedRerolled the patch, removing the webkit only directive as suggested in #65.
Comment #67
sqndr CreditAttribution: sqndr commentedSee #65.
Comment #68
vermario CreditAttribution: vermario commentedRight! :) Removed the additional webkit only rule.
Comment #69
LewisNymanLooks like Alex's issues have been address, here's an interdiff from 63-68 to confirm.
Comment #71
BarisW CreditAttribution: BarisW commentedHere's a re-roll.
Comment #72
BarisW CreditAttribution: BarisW commentedHmm interdiff failed. But the only difference is from this:
to:
Comment #73
LewisNymanYep looks like DRUPAL_ROOT has been changed to this->root. That's the only difference
Comment #74
alexpottTo 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 CreditAttribution: vermario commentedI have added the requested template. How do we make a decision on this now?
Comment #76
LewisNymanNow that we have the template I think we can move it back to RTBC.
Comment #78
LewisNymanComment #79
lauriiiReroll after: #2377397: Themes should use libraries, not individual stylesheets
Comment #81
herom CreditAttribution: herom commentedFixing the reroll.
Comment #82
LewisNymanA quick manual test and all is well.
Comment #83
alexpottWhat 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 CreditAttribution: sqndr commentedFixed the issue from #83.
Update: Something went wrong. Check the next comment for the correct patch.
Comment #85
sqndr CreditAttribution: sqndr commentedSomething went wrong. Here's the patch and the interdiff.
Comment #86
LewisNyman@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
alexpottCommitted 7aef0c7 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.
We should be changing the modes of these files - fixed on commit.
Comment #90
hass CreditAttribution: hass commentedThis patch is not complete.
Comment #91
mgifford