Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces optional lightweight markup parsing for node labels in the consort diagram package (covering grid rendering and Graphviz export), adding a parse_markup default option and updating measurement/rendering so formatted labels size and align correctly.
Changes:
- Added a new
parse_markupoption to the global defaults (set_consort_defaults()/consort_opt()) and documented it across README/vignette/man pages. - Implemented markup parsing + HTML-label conversion utilities and integrated them into Graphviz label generation (
mk_text_align()). - Updated grid textbox measurement/rendering to support mixed-style segments (bold/italic/super/sub/underline), and added tests/snapshots/examples.
Reviewed changes
Copilot reviewed 12 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/consort_diagram.Rmd | Adds documentation for the new parse_markup option and a markup usage example. |
| tests/testthat/test-markup.R | Adds unit + smoke tests for markup parsing, grid rendering, and Graphviz HTML label generation. |
| tests/testthat/_snaps/build_grviz/end-miss-grviz.png | Updates snapshot artifact for Graphviz output. |
| README.Rmd | Updates examples to enable markup parsing and adds grid usage. |
| README.md | Regenerated README output reflecting new examples and figure output. |
| R/textbox.R | Adds formatted-text measurement and grid rendering for markup segments. |
| R/markup.R | Introduces markup parsing, newline splitting, style-to-gpar mapping, and Graphviz HTML conversion. |
| R/grviz_util.R | Uses HTML-like Graphviz labels when markup is detected. |
| R/grid_util.R | Fixes padding calculation in calc_y_coords() and guards gp_consecutive() for length-1 input. |
| R/defaults.R | Adds parse_markup to defaults + validation + roxygen docs. |
| R/build_grid.R | Improves label X scaling and simplifies grob list construction. |
| NEWS.md | Notes new markup support (needs a small accuracy update). |
| man/set_consort_defaults.Rd | Documents the new parse_markup parameter. |
| man/figures/README-diagram-1.png | Updates README figure output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Each segment is list(text = "...", style = "plain"|"bold"|"italic"|...) | ||
| #' @keywords internal | ||
| parse_markup <- function(text) { | ||
| if (is.null(text) || !nzchar(text)) { |
There was a problem hiding this comment.
parse_markup() errors for NA_character_ because nzchar(NA) returns NA, which makes the if (is.null(text) || !nzchar(text)) condition evaluate to NA and fail with “missing value where TRUE/FALSE needed”. Consider explicitly handling NA, non-character inputs, and length != 1 (similar to has_markup()) and returning a plain segment in those cases.
| if (is.null(text) || !nzchar(text)) { | |
| if (is.null(text) || !is.character(text) || length(text) != 1 || | |
| is.na(text) || !nzchar(text)) { |
| @@ -84,15 +88,14 @@ out <- consort_plot(data = df, | |||
| side_box = c("exc", "fow1", "fow2"), | |||
| allocation = "arm", | |||
| labels = c("1" = "Screening", "2" = "Randomization", | |||
| "5" = "Final"), | |||
| cex = 0.6) | |||
| "5" = "Final")) | |||
|
|
|||
| plot(out) | |||
There was a problem hiding this comment.
The README output currently includes a #> message about defining cex via set_consort_defaults(...), which is confusing in documentation. Consider suppressing this in the chunk (e.g., message = FALSE) and/or explicitly passing cex = NULL to consort_plot() so the README stays focused on the example and doesn’t show deprecation noise.
| - Allow user configuration of arrow graphical parameters and padding with `set_consort_defaults()`. | ||
| - Allow custom bullet characters | ||
| - Allow custom bullet characters. | ||
| - Allow simple markup for bold, italic and superscript. |
There was a problem hiding this comment.
The NEWS entry says “Allow simple markup for bold, italic and superscript”, but this PR also adds subscript and underline (and a parse_markup option). Please update this bullet so it accurately reflects the supported markup and the new option name.
| - Allow simple markup for bold, italic and superscript. | |
| - Allow simple markup for bold, italic, superscript, subscript, and underline with `parse_markup`. |
| Markup can be mixed with plain text and line breaks (`\n`) as usual. | ||
|
|
||
|
|
||
| *Note*: the markup may not be perfect. There is known issue with Graphviz that the combination of bold and normal text may not render correctly and it may overlap with the surrounding text. If you encounter this issue, you can try to add some extra space around the styled text. |
There was a problem hiding this comment.
Grammar: “There is known issue with Graphviz …” should be “There is a known issue …” (and consider adding “a” before “known issue” for readability).
| *Note*: the markup may not be perfect. There is known issue with Graphviz that the combination of bold and normal text may not render correctly and it may overlap with the surrounding text. If you encounter this issue, you can try to add some extra space around the styled text. | |
| *Note*: the markup may not be perfect. There is a known issue with Graphviz that the combination of bold and normal text may not render correctly and it may overlap with the surrounding text. If you encounter this issue, you can try to add some extra space around the styled text. |
| test_that("parse_markup returns plain for empty/NULL", { | ||
| segs <- parse_markup("") | ||
| expect_equal(segs[[1]]$text, "") | ||
| expect_equal(segs[[1]]$style, "plain") | ||
|
|
||
| segs2 <- parse_markup(NULL) | ||
| expect_equal(segs2[[1]]$text, "") |
There was a problem hiding this comment.
Given parse_markup() currently doesn’t handle NA_character_ safely, it would be good to add a regression test here (e.g., expect_equal(parse_markup(NA_character_)[[1]]$style, "plain") or expect_no_error(parse_markup(NA_character_))) so this edge case stays covered.
| test_that("parse_markup returns plain for empty/NULL", { | |
| segs <- parse_markup("") | |
| expect_equal(segs[[1]]$text, "") | |
| expect_equal(segs[[1]]$style, "plain") | |
| segs2 <- parse_markup(NULL) | |
| expect_equal(segs2[[1]]$text, "") | |
| test_that("parse_markup returns plain for empty/NULL/NA", { | |
| segs <- parse_markup("") | |
| expect_equal(segs[[1]]$text, "") | |
| expect_equal(segs[[1]]$style, "plain") | |
| segs2 <- parse_markup(NULL) | |
| expect_equal(segs2[[1]]$text, "") | |
| expect_equal(segs2[[1]]$style, "plain") | |
| segs3 <- parse_markup(NA_character_) | |
| expect_equal(segs3[[1]]$text, "") | |
| expect_equal(segs3[[1]]$style, "plain") |
This pull request adds support for lightweight markup (bold, italic, superscript, subscript, underline) in node labels for the consort diagram package, controlled by a new
parse_markupoption. It also includes improvements to text measurement and rendering, especially for formatted labels, and various bug fixes and documentation updates.Markup support and rendering:
parse_markupoption toset_consort_defaults()andconsort_opt()to enable parsing of lightweight markup syntax (e.g.,**bold**,*italic*,^{superscript}) in node labels. [1] [2] [3] [4] [5]R/markup.R, including functions for parsing, splitting, and rendering markup, and converting to Graphviz HTML-like labels.grid.textboxand related functions to measure and render formatted (markup) text correctly, including handling for bold, italic, superscript, subscript, and underline, as well as multi-line and justified text. [1] [2] [3] [4] [5]Documentation and examples:
NEWS.mdand documentation to describe new markup features and options.README.Rmdshowing how to enable markup parsing and use formatted labels in a diagram. [1] [2]Bug fixes and minor improvements:
build_gridfunction parameter.build_gridfor more robust layout handling. [1] [2] [3] [4]calc_y_coordsand improved group assignment ingp_consecutive. [1] [2]