naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.03k stars 316 forks source link

Modify PrintPage to use new BrewRenderer methods #3464

Closed G-Ambatte closed 1 month ago

G-Ambatte commented 1 month ago

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.

5e-Cleric commented 1 month ago

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

G-Ambatte commented 1 month ago

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 commented 1 month ago

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

calculuschild commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

5e-Cleric commented 1 month ago

Issue #2742 tracks printing without print page

calculuschild commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

I am pulling new PR together for this now.

calculuschild commented 1 month ago

Closing in favor of #3491

Thanks @G-Ambatte !