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

Hotfix: change \newpage to \clearpage #216

Merged
merged 6 commits into from
Jul 30, 2024
Merged

Hotfix: change \newpage to \clearpage #216

merged 6 commits into from
Jul 30, 2024

Conversation

kelliemac
Copy link
Contributor

@kelliemac kelliemac commented Jul 30, 2024

Description

Yung-Wen noticed an issues with two figures being crammed onto the same page and running off the page when using the latest VISCtemplates to render a previous PT report. I was able to reproduce the issue and trace it to a limitation of the \newpage command that is now being used by insert_break(). Changing \newpage to \clearpage resolves the issue.

Helpful explanation of the differences between \newpage and \clearpage: https://tex.stackexchange.com/questions/497746/comparison-between-newpage-clearpage-and-pagebreak-etc

Checklist

  • This PR includes unit tests
  • This PR establishes a new function or updates parameters in an existing function
    • The roxygen skeleton for this function has been updated using devtools::document
  • I have updated NEWS.md to describe the proposed changes

@kelliemac kelliemac marked this pull request as ready for review July 30, 2024 18:25
@kelliemac kelliemac requested a review from slager July 30, 2024 18:25
@slager slager changed the base branch from main to develop July 30, 2024 20:04
@slager
Copy link
Contributor

slager commented Jul 30, 2024

After updating the templates to use \clearpage, I can see that using \clearpage appears to make the page breaks non-functional in Word output.

\newpage was supposed to work for everything, see here:
https://bookdown.org/yihui/rmarkdown-cookbook/pagebreaks.html#pagebreaks

@slager
Copy link
Contributor

slager commented Jul 30, 2024

Trying it conditionally depending on output type. This works for me locally @kelliemac. If this also works on CI, then maybe the only thing still needed here is updating the instructions in the vignette before doing a patch release.

@slager
Copy link
Contributor

slager commented Jul 30, 2024

Kellie was able to successfully test that this fixes the broken report in question, and I have added instructions to the vignette.

@kelliemac kelliemac merged commit 593a679 into develop Jul 30, 2024
6 checks passed
@kelliemac kelliemac deleted the fix-newpage-issue branch July 30, 2024 22:07
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.

2 participants