-
Notifications
You must be signed in to change notification settings - Fork 55
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
Conversation
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" /> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@@ -1,120 +0,0 @@ | |||
<template> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 || '') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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}`, { |
There was a problem hiding this comment.
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
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 notpdfGenerator.js
. The reason is, that I believe at this stagepaged.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.