Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Package: gglite
Title: Lightweight Data Visualization via the Grammar of Graphics
Version: 0.0.10
Version: 0.0.11
Authors@R: person("Yihui", "Xie", role = c("aut", "cre"), email = "xie@yihui.name",
comment = c(ORCID = "0000-0003-0645-5666"))
Description: A lightweight R interface to the AntV G2 JavaScript visualization
Expand Down
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ with support for 35+ geometry types (marks), scales, coordinates, themes,
transforms, facets, animations, and chart components (axes, legends, titles,
tooltips, labels, sliders, and scrollbars). Renders in R Markdown, litedown,
Shiny, and standalone HTML previews.

- Added `title` and `subtitle` arguments to `g2()` for setting the chart title
and subtitle directly, as a convenient alternative to piping into `title_()`.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've said this many times and written it in copilot instructions: DON'T INCLUDE NEWS FOR v0.1! (please emphasize this again in copilot instructions)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1c4c63a. Removed the NEWS entry and strengthened the copilot instructions in both the validation steps and conventions sections to explicitly say: do NOT add NEWS entries while the package is at v0.1.

14 changes: 12 additions & 2 deletions R/gglite.R
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ g2_patches_cdn = 'https://cdn.jsdelivr.net/npm/@xiee/utils@v1.14.30/js/g2-patche
#' (applied to all sides) or a length-4 vector `c(top, right, bottom, left)`;
#' use `NA` to skip individual sides. `NULL` (the default) leaves the value
#' unset.
#' @param title Chart title string, a convenient alternative to piping into
#' [title_()] separately.
#' @param subtitle Chart subtitle string. Only used when `title` is also
#' provided.
#' @return A `g2` object (S3 class).
#' @import stats utils
#' @export
Expand All @@ -66,9 +70,13 @@ g2_patches_cdn = 'https://cdn.jsdelivr.net/npm/@xiee/utils@v1.14.30/js/g2-patche
#' # Time series
#' g2(sunspot.year)
#' g2(EuStockMarkets)
#'
#' # Title and subtitle
#' g2(mtcars, hp ~ mpg, title = 'Motor Trend Cars', subtitle = 'mpg vs hp')
g2 = function(
data = NULL, ..., width = 640, height = 480,
padding = NULL, margin = NULL, inset = NULL
padding = NULL, margin = NULL, inset = NULL,
title = NULL, subtitle = NULL
) {
dots = list(...)
has_formula = length(dots) && inherits(dots[[1]], 'formula')
Expand Down Expand Up @@ -99,7 +107,9 @@ g2 = function(
theme = NULL,
axes = list(),
legends = list(),
chart_title = NULL,
chart_title = if (!is.null(title)) {
if (!is.null(subtitle)) list(title = title, subtitle = subtitle) else title
},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are so many is.null() tests in this package; can you remove as many of them as possible? in this specific case, you can simply do

Suggested change
chart_title = if (!is.null(title)) {
if (!is.null(subtitle)) list(title = title, subtitle = subtitle) else title
},
chart_title = list(title = title, subtitle = subtitle),

and drop the NULLs at some point

i don't like this pattern in build_config:

  if (!is.null(chart$chart_title)) config$title = chart$chart_title
  if (!is.null(chart$tooltip_config)) config$tooltip = chart$tooltip_config
  if (!is.null(chart$sliders)) config$slider = chart$sliders
  if (!is.null(chart$scrollbars)) config$scrollbar = chart$scrollbars

there's too much repetition, and even if you remove the !is.null() test, the code still works (assigning NULL to a list item will remove the item)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1c4c63a. Simplified chart_title to dropNulls(list(title = title, subtitle = subtitle)). Removed the is.null() checks for tooltip_config, sliders, and scrollbars in build_config() — now just direct assignments.

facet = facet_from_formula,
layout = c(
process_layout('padding', padding),
Expand Down
13 changes: 12 additions & 1 deletion man/g2.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions tests/testit/test-gglite.R
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,15 @@ assert('g2() formula ~ x1 + x2 + x3 sets position encoding', {
c('Sepal.Length', 'Sepal.Width', 'Petal.Length'))
(is.null(chart$aesthetics$x))
})

assert('g2() title argument sets chart title', {
chart = g2(mtcars, x = 'mpg', y = 'hp', title = 'My Title')
(chart$chart_title %==% 'My Title')
})

assert('g2() title and subtitle arguments set title and subtitle', {
chart = g2(mtcars, x = 'mpg', y = 'hp',
title = 'Title', subtitle = 'Subtitle')
(chart$chart_title$title %==% 'Title')
(chart$chart_title$subtitle %==% 'Subtitle')
})
Loading