Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the consort package’s configuration surface by introducing/standardizing global options for text/box styling, arrow styling, padding, and bullet characters, and aligns documentation/vignettes/tests with the new option names and features.
Changes:
- Introduces new configurable options (e.g.,
consort_arrow_gp,consort_pad_u,consort_bullet) and renames text/box style options toconsort_txt_gp/consort_box_gp. - Adds rotation support to
textbox()via a newleftrotateparameter. - Updates documentation (Rd/NEWS/vignette) and unit tests to reflect the new configuration interface.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/consort_diagram.Rmd | Documents new global options; updates examples to use consort_txt_gp. |
| tests/testthat/test-textbox.R | Updates option names used in tests for textbox styling. |
| R/utils.R | Adds sync_gpar() helper to coordinate gpar fill/col behavior. |
| R/textbox.R | Renames option lookups to consort_* and adds leftrotate support. |
| R/grid_util.R | Adds validation for consort_pad_u. |
| R/consort_plot.R | Updates to set consort_txt_gp from cex. |
| R/connect_box.R | Replaces lwd/col with arrow_gp and uses sync_gpar() for connector styling. |
| NEWS.md | Adds 1.2.3 release notes. |
| man/textbox.Rd | Updates docs for consort_txt_gp/consort_box_gp and adds leftrotate. |
| man/gen_text.Rd | Documents configurable bullet character support. |
| man/consort-package.Rd | Adds alias and useful links/author metadata. |
| man/consort_plot.Rd | Clarifies cex overriding global text gp option. |
| man/connect_box.Rd | Updates signature/docs for arrow_gp/arrow_length/arrow_type (but examples need fixes). |
| DESCRIPTION | Updates RoxygenNote. |
Comments suppressed due to low confidence (1)
R/connect_box.R:23
- The roxygen docs still refer to older styling controls (line width/color) and describe
arrow_lengthas aunitwith aunit(0.1, "inches")default, but the function signature defaults to numeric0.1and the API is now centered onarrow_gp. Please update the parameter docs (and the examples later in this block) so they match the currentconnect_box()arguments/options (consort_arrow_gp,consort_arrow_length, etc.).
#' @param arrow_gp Graphical parameters for the connector line. Defaults to
#' \code{grid::gpar(col = "black", lwd = 1)}.
#' @param arrow_length Length of the arrowhead as a \code{\link[grid]{unit}} object.
#' Defaults to \code{getOption("consort_arrow_length", default = unit(0.1, "inches"))}.
#' @param arrow_type Arrow type, either \code{"closed"} or \code{"open"}. Defaults to
#' \code{getOption("consort_arrow_type", default = "closed")}.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #' @param cex Multiplier applied to font size, Default is 0.8. This will override | ||
| #' the global option of \code{"consort_txt_gp"} if defined. |
There was a problem hiding this comment.
This option rename appears incomplete: other package code still reads getOption("txt_gp") (e.g., R/add_label_box.R:50), so setting consort_txt_gp here may not affect all text sizing/formatting as intended. Consider updating internal consumers to the new consort_txt_gp option (or supporting both option names for backward compatibility).
| @@ -29,8 +29,8 @@ test_that("Box options", { | |||
| expect_equal(bx1$box_gp$fill, "red") | |||
|
|
|||
| options( | |||
| txt_gp = gpar(), | |||
| box_gp = gpar() | |||
| consort_txt_gp = gpar(), | |||
| consort_box_gp = gpar() | |||
| ) | |||
There was a problem hiding this comment.
This test sets global options and then leaves them set to gpar() at the end, which can affect subsequent tests (because getOption("consort_txt_gp") will no longer fall back to the documented defaults). Use withr::local_options() or save/restore the previous values (and unset the options when they were originally NULL).
| connect, | ||
| type = c("s", "p"), | ||
| name = NULL, | ||
| arrow_gp = getOption("consort_arrow_gp", default = gpar(color = "black", lwd = 1)), |
There was a problem hiding this comment.
arrow_gp is shown as gpar(color = "black", ...) in the usage, but the argument documentation (and most grid code) uses col. For consistency (and to ensure sync_gpar() can see gp$col), update the default to gpar(col = "black", lwd = 1) and reflect that consistently in the Rd.
| arrow_gp = getOption("consort_arrow_gp", default = gpar(color = "black", lwd = 1)), | |
| arrow_gp = getOption("consort_arrow_gp", default = gpar(col = "black", lwd = 1)), |
| leftrotate = FALSE, | ||
| box_fn = roundrectGrob, | ||
| box_gp = getOption("box_gp", default = gpar(fill = "white")), | ||
| box_gp = getOption("consort_box_gp", default = gpar(fill = "white")), | ||
| name = "textbox") { | ||
| just <- match.arg(just) | ||
|
|
||
| if (!is.unit(x)) x <- unit(x, units = "npc") | ||
| if (!is.unit(y)) y <- unit(y, units = "npc") | ||
|
|
||
| angle <- ifelse(leftrotate, 90, 0) | ||
|
|
There was a problem hiding this comment.
leftrotate introduces new rendering behavior (rotated viewport via angle) but there’s no corresponding test coverage in tests/testthat/test-textbox.R to assert that the angle is applied (and that sizing/coords behave as expected). Consider adding a test that textbox(..., leftrotate=TRUE) stores angle=90 (and/or validates drawing/coords if feasible).
No description provided.