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 direct printing #2743

Closed
wants to merge 4 commits into from

Conversation

G-Ambatte
Copy link
Collaborator

@G-Ambatte G-Ambatte commented Mar 19, 2023

This PR resolves #2742.

The removal of the PrintPage and /print route have been left for a future PR, if their removal is deemed necessary.

Implementation Tasks:

  • Edit navbar/print.navitem.jsx to call the print function directly instead of linking to /print
    • As a React Function Component, rather than Class Component (optional?)
  • Add @media print CSS to BrewRenderer styling
  • Update NewPage to use the standard PrintNavItem rather than a custom LocalPrintNavItem
  • Test Print button functionality on all pages
    • EditPage
    • SharePage
    • NewPage

@G-Ambatte G-Ambatte added feature P2 - minor feature or not urgent Minor bugs or less-popular features labels Mar 19, 2023
@G-Ambatte G-Ambatte self-assigned this Mar 19, 2023
@G-Ambatte G-Ambatte marked this pull request as ready for review March 19, 2023 06:42
@G-Ambatte G-Ambatte requested a review from calculuschild March 19, 2023 06:42
@G-Ambatte
Copy link
Collaborator Author

After going back and forth and re-writing this from scratch, I eventually realized that the PrintNavItem (previous PrintLink) was actually already written as a Function Component.

@calculuschild calculuschild added P3 - low priority Obscure tweak or fix for a single user and removed P2 - minor feature or not urgent Minor bugs or less-popular features feature labels Mar 19, 2023
@G-Ambatte
Copy link
Collaborator Author

Possibly fatal issue:

When Partial Page Rendering is active, the content of the unrendered pages literally does not exist and thus cannot be passed to the printer.

@5e-Cleric
Copy link
Member

As pointed out in gitter a possible solution to the Partial Page Rendering(PPR) problem would be to create a temporary iframe that just loads the existing /page to then render the whole document for printing.

It could possible even be hidden visually from the user.

@calculuschild calculuschild added the 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure label Mar 23, 2023
Copy link
Member

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

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

I'm not sure I see the problem this is trying to solve. Can you elaborate?
/print is being used for other links. For instance it's what Google is using to track several of our reported issues on Chrome.

@calculuschild calculuschild temporarily deployed to homebrewery-pr-2743 April 24, 2023 18:53 Inactive
@5e-Cleric
Copy link
Member

5e-Cleric commented May 3, 2023

Possibly fatal issue:

When Partial Page Rendering is active, the content of the unrendered pages literally does not exist and thus cannot be passed to the printer.

image

this is the result, zoomed to see the icon in the top left of the page. all pages are printed, but those not loaded have the loading icon.

55 pages document, only 4 printed correctly.

@calculuschild
Copy link
Member

I think I'll close this issue for now. I'm not sure the issue this is trying to solve actually needs to be solved. Further discussion can happen on that open issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 - low priority Obscure tweak or fix for a single user 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print directly from pages, rather than via PrintPage
3 participants