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

Modify PrintPage to use new BrewRenderer methods #3464

Closed

Conversation

G-Ambatte
Copy link
Collaborator

This PR resolves #3435.

This PR modifies the Print Page to use the BrewRenderer to render the pages for printing. This ensures that the Print Page output will always match the Share and Edit Page visual outputs - previously it used a duplicated section of code that was not updated when the BrewRenderer was changed.

Some modifications have also been made to the BrewRenderer, including the option to not render inside an iFrame (defaults to iFrame on), and a method to indicate to Print Page when the print event should fire (i.e. on completion of document rendering).

iFrame rendering would be preferable but while it is possible to create functions to print the iFrame content directly, I have been unable to get printing via Ctrl+P or selecting Print from the browser menu to work correctly. For this reason, the brew renders outside of a frame on Print Page.

@G-Ambatte G-Ambatte self-assigned this May 9, 2024
@G-Ambatte G-Ambatte added bug We say this works but it doesn't P1 - high priority Obvious bug or popular features 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels May 9, 2024
@5e-Cleric
Copy link
Member

will review this as soon as i get my hands on a computer

@calculuschild calculuschild added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels May 13, 2024
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3464 May 16, 2024 23:24 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3464 May 16, 2024 23:39 Inactive
@@ -152,7 +162,7 @@ const BrewRenderer = (props)=>{
renderedPages[props.currentEditorPage] = renderPage(rawPages[props.currentEditorPage], props.currentEditorPage);

_.forEach(rawPages, (page, index)=>{
if((isInView(index) || !renderedPages[index]) && typeof window !== 'undefined'){
if((props.renderAllPages || isInView(index) || !renderedPages[index]) && typeof window !== 'undefined'){
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realized that using !props.frame as a check to render a page was particularly opaque, so I have introduced a new prop renderAllPages to make it clearer that all pages will be rendered from the Print Page.

@G-Ambatte
Copy link
Collaborator Author

I'm getting a weird case where headings are not rendered at the time the print dialog is first called, so they are missing from the output.
image

However, closing the print dialog and pressing Ctrl+P or even reloading the page works fine.
image

I'm still digging into it.

@5e-Cleric
Copy link
Member

Serious question, why did we agree on keeping this instead of printing from /edit or /share?

@calculuschild
Copy link
Member

calculuschild commented May 17, 2024

Serious question, why did we agree on keeping this instead of printing from /edit or /share?

I'm not sure I understand the question. Printing is possible from both /edit and /share via the "get PDF" button.

Printing HTML to PDF prints the entire page, so you need to somehow generate a separate window/frame. As far as I know you can't just send a single element to the printer.

@5e-Cleric
Copy link
Member

5e-Cleric commented May 17, 2024

Printing HTML to PDF prints the entire page, so you need to somehow generate a separate window/frame. As far as I know you can't just send a single element to the printer.

try this:

In the brew style tab:

@media print {
	.brewRenderer {
		height: 100%;
		overflow-y: unset;
		.pages {
			margin: 0px;
			&>.page {
			box-shadow: unset;
			}
		}
	}
}

In the dev tools console:

window.frames['BrewRenderer'].contentWindow.print()

@calculuschild
Copy link
Member

Well if that works, that would simplify things a lot. Does it still work with variables across pages? Work with Legacy brews? I'm ok doing that if we can eliminate the whole /print page.

@5e-Cleric
Copy link
Member

First one for sure, second haven't tried, should work fine

@5e-Cleric
Copy link
Member

Issue #2742 tracks printing without print page

@calculuschild
Copy link
Member

calculuschild commented May 17, 2024

Wow, I don't remember that issue at all, but I guess I was there. Looks like the main concern was keeping existing /print links working, but now that it's been a while I don't know how necessary that is. I used a few /print links to share minimal error examples with Google, but those ones have been solved. The other use case from @ericscheid was for the visualping tool which periodically scans a webpage for visual differences so you can be alerted to updates. It wouldn't work in /share because it doesn't automatically scroll through internal iFrames, but that also seems like visualping can handle that now.

@G-Ambatte
Copy link
Collaborator Author

As I recall, the print function didn't trigger a full render so PPR would break the print. I think that's no longer an issue with the changes to Brew Renderer, so the logic may no longer apply.

@calculuschild
Copy link
Member

Yeah, looks like the @media print { approach does work with variables and legacy brews. If we are in agreement, I say we close this PR and move instead to that as a simpler alternative.

@calculuschild
Copy link
Member

I am pulling new PR together for this now.

@calculuschild
Copy link
Member

Closing in favor of #3491

Thanks @G-Ambatte !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug We say this works but it doesn't P1 - high priority Obvious bug or popular features 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-page variable use doesn't work when parsed to PDF
3 participants