Skip to content

RS-22447: Export grid line type to editable PPT chart settings - #90

Merged
chschan merged 4 commits into
masterfrom
RS-22447-grid-line-type-ppt-export
Jul 2, 2026
Merged

RS-22447: Export grid line type to editable PPT chart settings#90
chschan merged 4 commits into
masterfrom
RS-22447-grid-line-type-ppt-export

Conversation

@chschan

@chschan chschan commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

RS-22447 — Grid line type in PowerPoint/Excel export

RS-22447

Companion to flipStandardCharts#130, which made the Grid line type control (Solid/Dot/Dash) work on-screen. This fixes the export path so exported (PPT/Excel) charts reflect it too.

Problem

getPPTSettings() hardcoded the MajorGridLine Style for both axes to "Solid" (or "None" when the grid width is 0), ignoring the grid dash setting.

Fix

Derive the style from values.grid.dash / categories.grid.dash using the existing getLineStyle() helper. It already:

  • returns "None" for a zero-width line, and
  • falls back to "Solid" when no dash is supplied,

so behaviour is unchanged when the control isn't present. getPPTSettings() runs before the categories/values → x/y rename, so args$values.grid.dash / args$categories.grid.dash are the correct names here (same as the existing zero-line-dash handling nearby).

Tests

  • test-chartsettings.R — new case asserts Dot/Dash reach ValueAxis/PrimaryAxis MajorGridLine$Style, that a zero-width grid stays "None", and that omitting the dash still yields "Solid". Full file: PASS 138 (with flipStandardCharts 1.32.12).

🤖 Generated with Claude Code

The MajorGridLine Style for both axes was hardcoded to "Solid" (or "None"
when width is 0), so the Grid line type control (Solid/Dot/Dash) was not
reflected in exported charts even after the on-screen fix.

Derive the style from values.grid.dash / categories.grid.dash via the
existing getLineStyle() helper, which already returns "None" for a
zero-width line and falls back to "Solid" when no dash is supplied (so
existing behaviour is unchanged when the control is absent).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@chschan
chschan requested a review from JustinCCYap July 2, 2026 03:53
@chschan chschan changed the title RS-22447 Export grid line type to PowerPoint/Excel chart settings RS-22447: Export grid line type to editable PPT chart settings Jul 2, 2026
@JustinCCYap

Copy link
Copy Markdown
Contributor

Review note: omitted grid.width now exports Style = "None" instead of "Solid"

getPPTSettings() receives user.args (the raw list(...)) at R/cchart.R:356, before the chart-function defaults are applied, so args$values.grid.width / args$categories.grid.width are NULL whenever the caller doesn't pass them explicitly.

  • Old: if (isTRUE(args$...grid.width == 0)) "None" else "Solid"NULL width yields isTRUE(NULL == 0)FALSE"Solid".
  • New: getLineStyle(list(width = args$...grid.width, ...))is.null(line$width)TRUE"None".

So exporting e.g. a Column chart (default y.grid.width = 1, grid visible on-screen) without explicitly passing values.grid.width flips the exported MajorGridLine$Style from "Solid" to "None" — the grid disappears in the PPT/Excel chart and diverges from the on-screen render, the opposite of this PR's fidelity goal. (Masked in the Displayr plugin flow where width is passed explicitly; direct R/API callers are exposed.) The existing tests always pass grid.width explicitly, so this path is uncovered.

Related: getLineStyle also throws on an NA width (if (NA) → "missing value where TRUE/FALSE needed"), where the old isTRUE(...) was NA-safe.

Recommendation: guard the width branch so an absent/NA width preserves the previous "Solid" default. For example:

Style = if (isTRUE(args$values.grid.width == 0)) "None"
        else getLineStyle(list(width = args$values.grid.width %||% 1,
                               dash = args$values.grid.dash))

(or have getLineStyle treat NULL/NA width as unspecified → "Solid" at this call site), and add a test for the omitted-grid.width case asserting "Solid".

Minor: .gitignore adds .positai but not .claude, whereas .Rbuildignore ignores both — consider adding .claude to .gitignore for consistency.

Comment thread .gitignore Outdated
# emacs file
*~ No newline at end of file
*~
.positai

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this and the .Rbuildignore‎ changes need to be added?

@chschan
chschan force-pushed the RS-22447-grid-line-type-ppt-export branch from 656f075 to 17e509d Compare July 2, 2026 05:13
Review follow-up. getPPTSettings() sees the raw user args before chart
defaults are applied, so grid.width is NULL when a caller doesn't pass it
(e.g. direct R/API callers of a chart with a default grid). Routing that
through getLineStyle() flipped the exported MajorGridLine Style from
"Solid" to "None" (grid vanished), and an NA width threw.

Add getGridLineStyle(width, dash): only an explicit width of 0 hides the
grid; a missing/NA width keeps the previous "Solid" default (NA-safe via
isTRUE), while an explicit dash still maps to Dot/Dash. Add a test for the
omitted-width case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@JustinCCYap JustinCCYap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

The regression escaped because every existing chart-settings test passed
grid.width explicitly, leaving the default/omitted path uncovered. Add a
direct unit test of getGridLineStyle spanning width 0/positive/NULL/NA x
dash present/absent, locking in the NULL/NA-safe "Solid" default the
reviewer flagged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@chschan
chschan merged commit ae18706 into master Jul 2, 2026
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants