Fix #333: allow showSelected.legend=FALSE to keep explicit showSelected without legend auto-injection#336
Fix #333: allow showSelected.legend=FALSE to keep explicit showSelected without legend auto-injection#336ANAMASGARD wants to merge 5 commits into
Conversation
Adds a WorldBank viz that keeps showSelected="year" but opts out of auto-adding region from the colour legend via showSelected.legend=FALSE. Fails on master because error_for_showSelected_variants() rejects the new parameter. #333
…lected only Whitelist showSelected.legend in error_for_showSelected_variants(), skip legend variable auto-injection in addShowSelectedForLegend() when FALSE, and strip the param in getLayerParams() so it never reaches exported data. #333
|
Sir @tdhock, I’ve implemented the fix for issue :
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #336 +/- ##
==========================================
+ Coverage 69.14% 73.03% +3.89%
==========================================
Files 163 164 +1
Lines 6002 8842 +2840
==========================================
+ Hits 4150 6458 +2308
- Misses 1852 2384 +532
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
it seems reasonable to create a new argument, |
|
Yes, that is my understanding as well. I’ll consolidate on showSelected.legend=FALSE as the explicit opt-out, remove the special handling for showSelected=character(), migrate the recent tests to the new API, and note the API cleanup in NEWS. |
| invalid_showSelected <- grep("^showSelected.+", names(params), value = TRUE) | ||
| invalid_showSelected <- setdiff( | ||
| grep("^showSelected.+", names(params), value = TRUE), | ||
| "showSelected.legend" |
There was a problem hiding this comment.
I don’t think this is necessary. Can you please revert?
| }) | ||
| test_that("showSelected=character() no longer opts layer out of legend injection", { | ||
| viz_char_ss <- list( | ||
| plot1 = ggplot(test_data, aes(x, y, color = comparison)) + |
There was a problem hiding this comment.
use informative name instead of number (plot1)

Summary
Fixes #333
PR #292 added
showSelected = character()so a layer can opt out of all auto legendshowSelectedbehavior while keeping the legend visible. Issue #333 asks for a narrower case: keep an explicitshowSelected(e.g. filter byyear) but do not auto-addshowSelectedfor colour/fill legend variables on the same layer (e.g.regionfromcolour = region).What was wrong?
On master, if you write
geom_point(..., colour = region, showSelected = "year"), animint2 adds bothyearandregionto the layer’sshowSelectedaesthetics. There was no way to suppress only the legend-driven part while keeping your ownshowSelected.What this PR does
Commit 1 (this push): failing test only — per CONTRIBUTING
tests/testthat/test-renderer2-issue333-opt-out-ss-with-other-showSelected.Rgeom(..., showSelected = "year", showSelected.legend = FALSE)showSelectedis only"year"(not"region")regionis still presentyearselector stayssinglefor animation / tallrectanimint2dir()becauseerror_for_showSelected_variants()does not whitelistshowSelected.legendyetCommit 2 (next): implementation
showSelected.legendinerror_for_showSelected_variants()(R/z_animintHelpers.R)addShowSelectedForLegend(), whenisFALSE(extra_params$showSelected.legend), skip appending legend variables toshowSelected(still honor user-specifiedshowSelected)showSelected.legendingetLayerParams()so it never appears in TSV/JSON exportExpected result
Users can write: