Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add print styles #931

Merged
merged 4 commits into from
Mar 15, 2024
Merged

add print styles #931

merged 4 commits into from
Mar 15, 2024

Conversation

FahrJo
Copy link
Contributor

@FahrJo FahrJo commented Mar 14, 2024

This PR addresses #107.
It was tested with Firefox 123.0.1, Chromium 116.0 and Safari 17.3. There sometimes occur minor glitches on Chromium and Safari but Firefox works well with my sample tables and views.

@juliusknorr juliusknorr requested review from enjeck and elzody March 14, 2024 07:18
@enjeck
Copy link
Contributor

enjeck commented Mar 14, 2024

Hi @FahrJo. Can you please explain what this fixes? There are no details at #107 either. If possible, please attach before/after images

@FahrJo
Copy link
Contributor Author

FahrJo commented Mar 14, 2024

Hi @enjeck, of course I can explain. Previously, the print view included the side bar as well as some control elements. Additionally only the first page was filled and not scaled to the size of the media at all.

Firefox 123.0.1 print dialog before the fix Firefox 123.0.1 print dialog after the fix

I also attached some sample printouts as PDF (yes, the Safari output was blank before the fix). The glitches I mentioned above are the untidy row breaks at the page breaks in the Chromium pdfs for instance (the collectives app has the same issues with page breaks btw.).

tables_print_chromium116_after_view.pdf
tables_print_chromium116_after.pdf
tables_print_chromium116_before_view.pdf
tables_print_chromium116_before.pdf
tables_print_firefox123_after_view.pdf
tables_print_firefox123_after.pdf
tables_print_firefox123_before_view.pdf
tables_print_firefox123_before.pdf
tables_print_safari17_after_view.pdf
tables_print_safari17_after.pdf
tables_print_safari17_before_view.pdf
tables_print_safari17_before.pdf

The views are exported as landscaped page to demonstrate the scaling and appearance for wide tables. Tell me if you need more demo material.

Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FahrJo Thank you. This is a great improvement!

the print view included the side bar as well as some control elements

Isn't this expected behavior? When I print a page, I would expect everything on the page to be printed, including the sidebar. I'll wait to hear others' opinion on this.

I'm also curious how it looks with big tables with the horizontal scrollbar.

@FahrJo
Copy link
Contributor Author

FahrJo commented Mar 14, 2024

Yes, that is the expected behavior with the default CSS but in common use cases, one only wants to print the table without any space lost for useless printed controls and menu sections. The collectives app is a great example for that same situation and already implements this. Also on many news pages or Wikipedia for example, if you print the page, the view is reduced to the main content and drops the GUI controls, adds, recommendations and comment sections.

I also add an example for a wide table:
Wide table print dialog
The content scales to the size of the page but I additionally adjusted the scaling setting in the print settings to avoid a line break. But there are some limits for content on a page for sure. In that situations (very wide tables) I would propose reducing the columns to print in a separate view of the table.

Veery wide table print dialog

@elzody
Copy link
Contributor

elzody commented Mar 14, 2024

I think the most expected behavior can be subjective, but when it comes to printing I think some UI elements should definitely be clipped. It's hard to say what a user might expect when they click the print button - maybe it's a matter of keeping the printing as-is when clicking the print button in the browser, but adding a print button to Tables could provide a way to print exclusively the table when desired, or tweak the behavior even further.

It may be difficult to come up with a solution for wide tables/columns, but I think the changes so far look okay. May require some further discussion, but I think you're on the right track.

Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool improvement. Thank you 😎

@enjeck
Copy link
Contributor

enjeck commented Mar 14, 2024

@FahrJo Please run npm run stylelint:fix and npm run lint:fix and commit. That should fix the stylelint CI checks.

@FahrJo
Copy link
Contributor Author

FahrJo commented Mar 14, 2024

I agree that there is room for improvement, but so far printing a table has not been possible at all. If I have the time, I can also add a button for this print view (in another PR). On the other hand, that would require consideration of the keyboard shortcut Ctrl+P. As a user, I would personally prefer the default print content to be the table/view only.

@elzody
Copy link
Contributor

elzody commented Mar 14, 2024

I agree that there is room for improvement, but so far printing a table has not been possible at all. If I have the time, I can also add a button for this print view (in another PR). On the other hand, that would require consideration of the keyboard shortcut Ctrl+P. As a user, I would personally prefer the default print content to be the table/view only.

That definitely makes sense. Couldn't hurt if this is something you feel like implementing would be good. If anything it would be nice to bring this issue to light some more, since I think it's important to consider. It's only natural some users would want to print tables, so defining the behavior of this is good

@enjeck enjeck force-pushed the enh/print-layout branch from bfaf452 to 08af97e Compare March 15, 2024 03:36
@enjeck enjeck merged commit 4d7a189 into nextcloud:main Mar 15, 2024
45 checks passed
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants