-
Notifications
You must be signed in to change notification settings - Fork 127
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
Upgrade paged.js to version 0.2.0 #252
base: main
Are you sure you want to change the base?
Conversation
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.
Hi @felipecrp,
Thanks a lot for this PR. We definitely should upgrade Paged.js to 0.2.0.
Usually, we upgrade Paged.js running the tools/update.R script. Sorry, this is undocumented. If we want to rename paged.js
to paged.polyfill.js
, we should update this script too.
I haven't upgraded Paged.js to 0.2.0 yet because it brings a native support for footnotes (see https://www.pagedjs.org/posts/2021-06-newrelease/) which is very cool.
However, this feature relies on a major change in the Paged.js page box model. From my experience, this kind of change can easilly break some templates. Before upgrading to 0.2.0, we should carefully test pagedown builtin templates (and maybe the pagedreport ones). Have you had time to test them?
While upgrading to Paged.js 0.2.0, I think we also should remove the footnotes support introduced by #21 and offer a new support for footnotes relying on Paged.js' native one. I can do it but I won't have the bandwith before december.
Hi @RLesur, I did just minor tests with my documents. I agree that more tests are required. I saw some unit tests on the project, but they apparently test some render features but do not assert whether pagedown created the output correctly. It would be nice to have automated regression tests, but I understand that this might be difficult because we need to compare the generated files visually. We need to think about how to create some tests to enable regression before major changes. Do you have any suggestions? Regards, |
@felipecrp Having the tools to perform visual tests against regressions would be great. The |
@RLesur some tests can also explore the generated DOM. For instance, a javascript code could check whether a header is repeated on every page or if a footnote is presented at the bottom of the page. |
I just saw that the paged.js use this strategy (DOM check) for unit testing. https://gitlab.pagedmedia.org/tools/pagedjs/tree/master/specs They basically have an HTML and a javascript file that check whether the result is ok. I think we could do the same, but with RMarkdown. |
@felipecrp You're right, DOM checking could be another strategy. However, I wonder whether this would be useful in the context of pagedown. For instance, in the context of the current PR, DOM checks would warn that the DOM have changed (because the Paged.js' page box model changes), this wouldn't be a great help. But feel free to send a proposal! |
@RLesur I think we only need to unit test the 'hacks' included on pagedown, e.g., the hooks or custom behaviors. |
Automated more tests would be awesome ! But testing R Markdown ecosystem is really not easy. Especially pagedown which acts at the client level with lots of JS in the browser.
We could also switch to a JS framework for that but it would need to run R in the first step and we would need to develop more JS skills. From paged.js test workflow:
I don't know if not using R as advantage here for testing R package. Last solution we already think about is a printing to PDF or image and doing a visual comparison but this has other challenges and if we detect an issue, it would need manual visual inspection for find the difference, then the part in the DOM, then the code that fail. Anyway, interesting topic as you see! 😄 but after all , I think this would still not prevent us to do some manual visual checks of our template and pagedreport template. Not everything change can be auto test. What we could improve right now is make a checklist, and some helper functions so that we can easily run manual test without much trouble and know what to look into. I believe this would be the first step before |
I was trying to set up the test environment yesterday, using JEST but got some issues with my WSL installation to use puppeteer. Since we need to test the DOM, I believe that it will be easier to use a JS test framework, such as JEST. Of course, we need to start from an Rmd file, then we need to use a workflow like the following:
Maybe, the easier way to accomplish this is to use testthat just to generate the temporary HTML files and JEST to consume the temporary HTML, assert, and delete. We just need to assert that testthat would run first. Also, we could try to use some JEST before assignments to call R, but I think this could be more complicated. PS: I think this discussion deserves another PR/issue because it is a much complex issue than the upgrade itself. Regards, |
FWIW testing the DOM content using xml2 and XPATH request is what we do in other tools to test that the created DOM is the one expected. Also chromote or crrri would be enough to do like puppeteer from R directly.
Agreed I share my thoughts here because I was under the impression you would have already tried something - and I was right 😉 Please feel free to open a new issue to share your idea, your progress and thoughts on all this. I don't know what the best solution would be. I just know this is not something pagedown specific and it is more broader problem. But trying something with pagedown is definitely worth it ! |
I upgraded the paged.js from version 0.1.43 to 0.2.0.
I noticed that we use the paged.polyfill.js instead of the paged.js file. Hence, I renamed the file to make future upgrades straightforward.
Regards,