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

chore(print-nuxt): use page.goto() to load print content #3024

Merged
merged 3 commits into from
Oct 8, 2022

Conversation

usu
Copy link
Member

@usu usu commented Oct 6, 2022

  • Improved error messages: instead of throwing a server error and aborting PDF generation, print the error onto the PDF and properly render the rest

  • Improved HTML content loading: instead of utilizing nuxt.renderRoute() and page.setContent(), load the content via a proper HTTP request page.goto(). This this passing of the jwt cookies, but I think is more robust and provides a better developer experience (no more waiting for nuxt build in development mode).

I only modified pdfGeneratorNativeChrome.js and not pdfGenerator.js. The reason is, that I believe at this stage paged.js will not provide the functionality we need. There is active development ongoing (more than ever) and the mattermost discussion is also decently active and questions are being responded to. There was also an attempt to improve table layout rendering. However, even with these improvements and the latest beta, it doesn't render the layout we're using properly and it seems a non-trivial problem to solve.

At the same time, native chrome does most of the stuff we'd need (with the biggest current exception being page numbers on the TOC). There is also active development of the print rendering ongoing: LayoutNGPrinting might ship with Chrome 108. In itself LayoutNGPrinting will not bring much improvements (main target is to replicate the previous behavior), but it's a door-opener for various old issues related to printing. Improved page breaking as an example is already implemented into LayoutNGPrinting.

So my proposal would be to remove paged.js (at least for the moment) and rely on native chrome to generate the pdf. This would directly imply removal of the developer print preview and offer PDF download only.

@@ -18,6 +19,10 @@ export default {
}
},
async fetch() {
if (this.options.scheduleEntry === null || this.options.activity === null) {
throw new Error('No activity and scheduleEntry provided provided')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new Error('No activity and scheduleEntry provided provided')
throw new Error('No activity and scheduleEntry provided')

@@ -1,7 +1,9 @@
<template>
<div>
<generic-error-message v-if="$fetchState.error" :error="$fetchState.error" />
Copy link
Member

Choose a reason for hiding this comment

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

Is there no way to avoid having to write this in every component? Maybe a (functional) component which can wrap any other component and display the fetch state error of the child component, if any? Then you could use that error component only in print/pages/index.vue and print/components/config/Toc.vue

Copy link
Member Author

Choose a reason for hiding this comment

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

Repeating was a bit the purpose of this: If fetching fails in a specific component, the error is printed for this specific component and the rest of this component not rendered (v-else). However, the remaining components would still render properly.

This makes it easier to detect where the problem is. And I believe is also a better user experience as the rest of the document would still render correctly and is theoretically still printable.

print/components/generic/ErrorMessage.vue Show resolved Hide resolved
@@ -1,120 +0,0 @@
<template>
Copy link
Member

Choose a reason for hiding this comment

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

Is deleting this also part of the pagedjs removal? Is there no way we will print without vuetify in the future? v-calendar for vuetify 3 is roughly at position 600 of 1004 in the column "New issues" (a column even more left than "Icebox"). I doubt that will be developed anytime soon...
https://app.zenhub.com/workspaces/vuetify-58dc72c478c79d245dc238f2/board
https://app.zenhub.com/workspaces/vuetify-58dc72c478c79d245dc238f2/issues/vuetifyjs/vuetify/13469

Copy link
Member Author

Choose a reason for hiding this comment

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

I think removal of pagedjs and printing without vuetify is 2 independent topics. Latter would need, as you said, a proper replacement of v-calendar.

@@ -31,39 +30,44 @@ function measurePerformance(msg) {
lastTime = now

console.log('\n')
console.log(msg)
console.log(msg || '')
Copy link
Member

Choose a reason for hiding this comment

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

console.log should be able to handle any (falsey) value you throw at it, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was printed as undefined

@carlobeltrame
Copy link
Member

I agree with the removal of pagedjs. But generally, how do we develop / debug print views without preview? If the best way is to open the nuxt page in a browser (and maybe resize the window, and activate print media simulation), should we document that somehow or add a convenience link somewhere for developers (like we have the convenience language switch in the footer which is not intended to go to production)?

@usu
Copy link
Member Author

usu commented Oct 7, 2022

I agree with the removal of pagedjs. But generally, how do we develop / debug print views without preview? If the best way is to open the nuxt page in a browser (and maybe resize the window, and activate print media simulation), should we document that somehow or add a convenience link somewhere for developers (like we have the convenience language switch in the footer which is not intended to go to production)?

Some sort of convencience link for developers sounds like a good proposal. Let us discuss quickly today, and if others agree, I'd make a separate PR with removal of pagedjs + some kind of developer preview link.

await page.setContent(html, { waitUntil: 'networkidle0' })

// HTTP request back to Print Nuxt App
await page.goto(`${process.env.PRINT_SERVER}/?config=${req.query.config}`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would try if localhost isn't enough

@usu usu merged commit 3ca151e into ecamp:devel Oct 8, 2022
@usu usu deleted the chore/print-nuxt-pdf-generation branch November 6, 2022 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants