Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comma-separated split of tmp.line.style ran for every chart type. Only Line supports per-series line type (Radar has no line.type parameter; other charts use a single style), and Radar/Time Series reach the per-series OutlineStyle loop - so a stray comma-separated line.type could wrongly apply per-series to them. Split only when chart.type == "Line"; still rep() to n series for all types. Mirrors how line.thickness's split is already scoped to its chart-type branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
chschan
changed the title
chschan
marked this pull request as draft
July 29, 2026 06:31
getPPTSettings gave every line chart a marker on every point, ignoring marker.show entirely, so an exported deck disagreed with the chart on screen wherever markers were only at the ends of a series. flipStandardCharts now reports the points showing a marker. Where that is not every point, the series turns its own marker off and those points switch it back on again, keeping the series' symbol. A list of points numbered across the whole chart is CombinedScatter's annotation borders, which say nothing about visibility, so it is left alone; so is a chart from a flipStandardCharts that reports nothing, which keeps the old behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chschan
marked this pull request as ready for review
July 31, 2026 04:38
Contributor
|
@chschan should I hold off on this since there will be more changes for series after this morning's standup? |
PowerPoint takes one smoothing setting for the whole chart, and it was worked out by comparing the shape argument against "Curved". Now that the shape can name one per series, that comparison reads "Curved,Curved" as no series being curved at all, so a chart drawn entirely with curves exported straight. The value is parsed first and the first series decides, which is the series the other whole-chart settings here are taken from. Curved is the name the controls send and spline is plotly's; the chart draws either as a curve, so the export now treats them alike. The numeric settings needed nothing: they already read a comma-separated string, a number, or one number per series, so charts saved from the old text box and from the new numeric controls both export the same. Tests cover that rather than leaving it to be rediscovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@JustinCCYap , I've updated this so it takes account of changes from this morning's description - added Shape parameter, can accept comma-separated strings or vectors. |
The pinned versions decided what was installed, so the version requirement in DESCRIPTION only judged the result: relaxing it could not help while CI was still fetching flipStandardCharts 1.32.4. Both pins had fallen a long way behind — master is 1.32.15 and 1.2.1 — and rhtmlCombinedScatter 1.0.14 no longer satisfies what flipStandardCharts itself asks for. They were there to build against companion versions that had not been merged yet. Both are merged, and DESCRIPTION already lists flipStandardCharts under Remotes, so with no pin the dependencies come from master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pinned versions decided what CI installed, so the requirement in DESCRIPTION only judged the result afterwards. Both pins had fallen well behind what the tests need: the per-point marker export reads a CustomPoints attribute that flipStandardCharts only reports from 1.32.14, and Radar's line type arrived around the same time, so building against 1.32.4 fails those tests whatever the requirement says. rhtmlCombinedScatter 1.0.14 also no longer satisfies what flipStandardCharts asks for. Both companions are merged, so the pins move up to their current versions rather than being dropped: emptying them made the build resolve a published flipStandardCharts older than the tests need. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR ensures that new controls implemented in Displayr/flipStandardCharts#132 are respected when exporting to Powerpoint.
Changes
getPPTSettings()now emits per-seriesOutlineStyle(line type),Marker$Size, andMarker$Style(marker symbol) for the editable-PowerPoint export, mirroring the existing per-seriesline.thickness/data.label.font.colorrecipe (ConvertCommaSeparatedStringToVector+rep(..., length=n)+[i]).markerSymbolToPPTStyle()maps plotly symbol names to PPT marker styles (circle→Circle,square→Square,diamond→Diamond,*-open→closest solid).CustomPointsattribute; where that is not every point, the series setsStyle = "None"and those points switch it back on. This makes the export respectmarker.showfor the first time, includingmarker.show.at.endsandmarker.show.at.last.end.Smoothis read from the line shape per series rather than by comparing the whole argument, so"Curved,Curved"no longer exports as straight. The first series decides, since PowerPoint takes one setting for the chart.splineis treated asCurved.CChartpassthrough to flipStandardCharts) is unchanged.Back-compat
The numeric settings accept every form a saved chart may carry: a comma-separated string from the old text box (
"6,10,14"), a single number or string from the new numeric controls, and one number per series. Charts with noCustomPointsattribute — anything built by a flipStandardCharts without that feature — keep the previous behaviour of a marker on every point.Known limitations
radarStyleis a single choice ofstandard,markerorfilled, and we export filled ("Radar Filled"→xlRadarFilled→RadarStyle.Filled). Filled and marker are alternatives, so markers could only be shown by giving up the polygon fill. Deliberately left as is.PptSeriesSettingshas noSmoothproperty, so the whole chart takes the first series' shape.PptXmlChartalready writes<c:smooth>inside each series element from the chart-level value, so honouring it per series would be a small change in q.Tests
1199 green across the flipChart suite. New tests cover per-series values, per-point markers, the
Smoothforms, and the input forms above.Cross-repo
Displayr/flipStandardCharts#132 and Displayr/rhtmlCombinedScatter#96 are merged; Plugins FS2-4532 carries the controls. Manual end-to-end verification in live Displayr is still outstanding, including whether the renderer honours a per-point
Style = "None", which the per-point marker export relies on.