Conversation
…aw_line and draw_scatter functions
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces DataZoom and MarkArea components, enabling interactive zooming and shaded background regions for line and scatter charts. It also adds support for explicit axis limits and toggling point markers. The review feedback focuses on enhancing robustness for empty datasets, specifically addressing potential out-of-bounds errors and the handling of non-finite values from range() calls. A design improvement was also suggested to ensure background annotations remain visible when data series are hidden.
| if (!is.null(mark_area)) { | ||
| series[[1]]@mark_area <- mark_area | ||
| } |
There was a problem hiding this comment.
This block will cause a "subscript out of bounds" error if y is an empty list, as series will also be empty. It's safer to check if series has at least one element before attempting to attach the mark_area.
if (!is.null(mark_area) && length(series) > 0L) {
series[[1]]@mark_area <- mark_area
}| # lines fill the plot width; users can pass `xlim`/`ylim` for explicit bounds. | ||
| y_all <- if (is.list(y)) unlist(y, use.names = FALSE) else y | ||
| x_lim <- if (x_type == "value") (xlim %||% range(x, na.rm = TRUE)) else NULL | ||
| y_lim <- ylim %||% range(y_all, na.rm = TRUE) |
There was a problem hiding this comment.
If y_all is empty (e.g., if y is an empty list), range(y_all, na.rm = TRUE) will throw an error or return c(Inf, -Inf). This will lead to a crash or invalid axis limits. It's better to handle the empty case explicitly to allow ECharts to perform automatic scaling.
y_lim <- ylim %||% (if (length(y_all) > 0L) range(y_all, na.rm = TRUE) else NULL)| if (!is.null(mark_area)) { | ||
| series[[1]]@mark_area <- mark_area | ||
| } |
There was a problem hiding this comment.
Attaching the mark_area only to the first series means that if the user hides that series via the legend, the background bands will also disappear. For background annotations that should persist regardless of which data series are visible, consider attaching the mark_area to all series or using a dedicated hidden series.
| min = y_lim[[1L]], | ||
| max = y_lim[[2L]] |
There was a problem hiding this comment.
If y_lim is NULL (which can happen if the data is empty and the suggestion for line 267 is applied), accessing y_lim[[1L]] will error. These properties should handle NULL values to allow ECharts to perform automatic scaling.
min = if (!is.null(y_lim)) y_lim[[1L]] else NULL,
max = if (!is.null(y_lim)) y_lim[[2L]] else NULL| if (is.null(value)) { | ||
| return(invisible(value)) | ||
| } | ||
| if (!is.numeric(value) || length(value) != 2L || anyNA(value)) { |
There was a problem hiding this comment.
The validation should also check for finite values. If range() is called on empty data, it returns c(Inf, -Inf), which are numeric and not NA, but are invalid as axis limits in ECharts.
if (!is.numeric(value) || length(value) != 2L || !all(is.finite(value))) {There was a problem hiding this comment.
Pull request overview
This PR introduces interactive zooming and axis-limit controls to the R ECharts wrapper, along with support for background block shading on line charts and new marker/component classes.
Changes:
- Add
DataZoomsupport (class +EChartsOption(data_zoom=...)serialization) and expose it viadraw_line(zoom=...). - Add
xlim/ylimhandling fordraw_line()anddraw_scatter(), pluspointsandblocks(markArea bands) fordraw_line(). - Expand test coverage and generated documentation for the new APIs/classes; remove and ignore
specs/docs.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/plan.md | Removed architecture plan document. |
| specs/goal.md | Removed goal document. |
| specs/draw_a3.md | Removed A3 design document. |
| specs/classes.md | Removed classes mapping document. |
| r/tests/testthat/test-widget.R | Added tests for draw_line() (points, blocks, zoom, xlim/ylim) and draw_scatter() limits. |
| r/tests/testthat/test-series.R | Added tests for MarkArea / MarkAreaDataPoint and LineSeries(mark_area=...). |
| r/tests/testthat/test-option.R | Added tests for EChartsOption(data_zoom=...) serialization behavior. |
| r/tests/testthat/test-components.R | Added tests for DataZoom validation and serialization. |
| r/man/draw_scatter.Rd | Documented new xlim/ylim parameters. |
| r/man/draw_line.Rd | Documented new points, blocks, block_*, xlim/ylim, and zoom parameters. |
| r/man/MarkAreaDataPoint.Rd | Added generated docs for MarkAreaDataPoint. |
| r/man/MarkArea.Rd | Added generated docs for MarkArea. |
| r/man/LineSeries.Rd | Documented new mark_area argument. |
| r/man/EChartsOption.Rd | Documented new data_zoom argument. |
| r/man/DataZoom.Rd | Added generated docs for DataZoom. |
| r/R/utils.R | Added validate_axis_lim() helper. |
| r/R/series.R | Added MarkAreaDataPoint/MarkArea classes and LineSeries(mark_area=...). |
| r/R/option.R | Added data_zoom property and dataZoom serialization logic. |
| r/R/draw.R | Implemented draw_line() points/blocks/xlim/ylim/zoom; implemented draw_scatter() xlim/ylim. |
| r/R/components.R | Added DataZoom class and supporting validators. |
| r/NAMESPACE | Exported DataZoom, MarkArea, MarkAreaDataPoint. |
| .gitignore | Added specs/ to ignored paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Resolve axis limits. Defaults apply 4% symmetric padding so points aren't | ||
| # drawn on the axis edges — matches base R `xaxs = "r"` and ggplot2 ~5%. | ||
| x_lim <- xlim %||% calc_limits(x) | ||
| y_lim <- ylim %||% calc_limits(y) |
There was a problem hiding this comment.
draw_scatter now forces x_axis$min/max and y_axis$min/max based on calc_limits(). For constant data where the range is exactly 0 (notably all zeros), calc_limits() currently returns identical min/max, which can lead to a collapsed axis range in ECharts. Consider ensuring the computed limits always have a positive span (either by fixing calc_limits()’s span==0 handling, or by guarding here before setting Axis(min/max)).
| # Resolve axis limits. Defaults apply 4% symmetric padding so points aren't | |
| # drawn on the axis edges — matches base R `xaxs = "r"` and ggplot2 ~5%. | |
| x_lim <- xlim %||% calc_limits(x) | |
| y_lim <- ylim %||% calc_limits(y) | |
| ensure_positive_span <- function(lim) { | |
| if (is.null(lim) || length(lim) != 2L || anyNA(lim)) { | |
| return(lim) | |
| } | |
| span <- lim[2] - lim[1] | |
| if (isTRUE(span > 0)) { | |
| return(lim) | |
| } | |
| center <- lim[1] | |
| delta <- max(abs(center) * 0.04, 1) | |
| c(center - delta, center + delta) | |
| } | |
| # Resolve axis limits. Defaults apply 4% symmetric padding so points aren't | |
| # drawn on the axis edges — matches base R `xaxs = "r"` and ggplot2 ~5%. | |
| x_lim <- xlim %||% ensure_positive_span(calc_limits(x)) | |
| y_lim <- ylim %||% ensure_positive_span(calc_limits(y)) |
| # lines fill the plot width; users can pass `xlim`/`ylim` for explicit bounds. | ||
| y_all <- if (is.list(y)) unlist(y, use.names = FALSE) else y |
There was a problem hiding this comment.
When all x (or all y) values are NA, range(..., na.rm = TRUE) returns c(Inf, -Inf), which then gets assigned to Axis(min/max) and will generate invalid ECharts options. Consider explicitly checking for at least one finite value before computing defaults, and erroring with a clear message if the data has no non-missing values.
| # lines fill the plot width; users can pass `xlim`/`ylim` for explicit bounds. | |
| y_all <- if (is.list(y)) unlist(y, use.names = FALSE) else y | |
| # lines fill the plot width; users can pass `xlim`/`ylim` for explicit bounds. | |
| has_finite_values <- function(values) { | |
| any(is.finite(values)) | |
| } | |
| y_all <- if (is.list(y)) unlist(y, use.names = FALSE) else y | |
| if (is.null(xlim) && x_type == "value" && !has_finite_values(x)) { | |
| cli::cli_abort( | |
| "Cannot derive default {.arg xlim} because {.arg x} has no non-missing finite values." | |
| ) | |
| } | |
| if (is.null(ylim) && !has_finite_values(y_all)) { | |
| cli::cli_abort( | |
| "Cannot derive default {.arg ylim} because {.arg y} has no non-missing finite values." | |
| ) | |
| } |
| # Resolve axis limits. Defaults apply 4% symmetric padding so points aren't | ||
| # drawn on the axis edges — matches base R `xaxs = "r"` and ggplot2 ~5%. | ||
| x_lim <- xlim %||% calc_limits(x) | ||
| y_lim <- ylim %||% calc_limits(y) |
There was a problem hiding this comment.
Similar to the constant-span case: if x or y are all NA, calc_limits() will compute range(..., na.rm=TRUE) as c(Inf, -Inf), and those invalid bounds are then forced onto the axes. Consider validating that x and y contain at least one finite value (or adjust calc_limits() to error on empty/non-finite ranges) before setting Axis(min/max).
| data_zoom_val <- x@data_zoom | ||
| if (!is.null(data_zoom_val)) { | ||
| if (S7::S7_inherits(data_zoom_val)) { | ||
| out$dataZoom <- list(to_list(data_zoom_val)) | ||
| } else if (is.list(data_zoom_val)) { | ||
| out$dataZoom <- lapply(data_zoom_val, function(dz) { | ||
| if (S7::S7_inherits(dz)) to_list(dz) else dz | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
If data_zoom is set to a non-list, non-DataZoom value, it is currently silently dropped (no dataZoom key is emitted). That makes misconfiguration hard to diagnose. Consider validating data_zoom and erroring via cli::cli_abort() when it’s neither a DataZoom nor a list (or add a property-level validator).
| \code{color} takes precedence over the theme palette (it sets \code{option.color}).} | ||
|
|
||
| \item{xlim}{Optional Numeric [length 2]: X-axis limits \code{c(min, max)}. | ||
| Ignored when \code{x} is not numeric. Defaults to \code{range(x)} (no padding) when |
There was a problem hiding this comment.
The documentation says xlim is “Ignored when x is not numeric”, but the implementation errors when xlim is provided on a category axis. Either update the docs to say it errors, or change the implementation to silently ignore xlim in that case for consistency with the Rd.
| Ignored when \code{x} is not numeric. Defaults to \code{range(x)} (no padding) when | |
| Only supported when \code{x} is numeric; providing \code{xlim} when \code{x} is not | |
| numeric results in an error. Defaults to \code{range(x)} (no padding) when |
| # ECharts treats missing `showSymbol` as TRUE; only pass FALSE when hiding. | ||
| show_symbol <- if (isTRUE(points)) NULL else FALSE |
There was a problem hiding this comment.
points is documented as logical, but show_symbol <- if (isTRUE(points)) NULL else FALSE will treat any non-TRUE value (including NA, a length>1 logical, or a non-logical value like "no") as “hide symbols” instead of erroring. Add an explicit validation that points is a single non-NA logical before computing show_symbol.
| # ECharts treats missing `showSymbol` as TRUE; only pass FALSE when hiding. | |
| show_symbol <- if (isTRUE(points)) NULL else FALSE | |
| if (!is.logical(points) || length(points) != 1L || is.na(points)) { | |
| cli::cli_abort("{.arg points} must be a single non-missing logical value.") | |
| } | |
| # ECharts treats missing `showSymbol` as TRUE; only pass FALSE when hiding. | |
| show_symbol <- if (points) NULL else FALSE |
| if (!is.numeric(block_opacity) || length(block_opacity) != 1L) { | ||
| cli::cli_abort("{.arg block_opacity} must be a single number.") | ||
| } |
There was a problem hiding this comment.
block_opacity is documented as in [0, 1], but validation only checks it’s a single number. Values like -1 or 2 will currently pass through to ItemStyle(opacity=...). Add an explicit range check and error message for out-of-bounds values.
| if (!is.numeric(block_opacity) || length(block_opacity) != 1L) { | |
| cli::cli_abort("{.arg block_opacity} must be a single number.") | |
| } | |
| if (!is.numeric(block_opacity) || length(block_opacity) != 1L || !is.finite(block_opacity)) { | |
| cli::cli_abort("{.arg block_opacity} must be a single finite number.") | |
| } | |
| if (block_opacity < 0 || block_opacity > 1) { | |
| cli::cli_abort("{.arg block_opacity} must be between 0 and 1, inclusive.") | |
| } |
| out$dataZoom <- lapply(data_zoom_val, function(dz) { | ||
| if (S7::S7_inherits(dz)) to_list(dz) else dz | ||
| }) |
There was a problem hiding this comment.
data_zoom is intended to serialize as an array, but if the user passes a named list (e.g., list(slider = DataZoom(...), inside = DataZoom(...))), the names are preserved here. With jsonlite::toJSON(), a named list becomes a JSON object, not an array, which will not match ECharts’ dataZoom: [...] format. Consider unname()-ing the list before assigning to out$dataZoom (similar to MarkArea’s data handling).
| out$dataZoom <- lapply(data_zoom_val, function(dz) { | |
| if (S7::S7_inherits(dz)) to_list(dz) else dz | |
| }) | |
| out$dataZoom <- unname(lapply(data_zoom_val, function(dz) { | |
| if (S7::S7_inherits(dz)) to_list(dz) else dz | |
| })) |
|
|
||
| # Determine level order so positional matching is stable. | ||
| levels_vec <- if (is.factor(blocks)) { | ||
| levels(blocks) |
There was a problem hiding this comment.
For factor blocks, levels_vec <- levels(blocks) includes unused levels; this makes positional matching and “missing color” checks depend on unused factor levels, which contradicts the docs (“k is the number of unique levels in blocks”). Consider deriving levels from the observed non-NA values (e.g., unique(as.character(blocks[!is.na(blocks)])) or levels(droplevels(blocks))) so unused levels don’t force extra block_color entries.
| levels(blocks) | |
| levels(droplevels(blocks)) |
… emphasize zero visibility
No description provided.