-
Notifications
You must be signed in to change notification settings - Fork 2.1k
get_layer_data(), get_layer_grob() can use layer names #6724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0287675
fa141b8
bae408c
cf688a5
8df2e9a
f9bfb44
bc143c3
5a877c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,6 +226,42 @@ test_that("layer_data returns a data.frame", { | |
| expect_snapshot_error(l$layer_data(mtcars)) | ||
| }) | ||
|
|
||
| test_that("get_layer_data works with layer names", { | ||
| p <- ggplot() + geom_point(name = "foo") + geom_point(name = "bar") | ||
|
|
||
| # name has higher precedence than index | ||
| expect_identical( | ||
| get_layer_data(p, i = "bar"), | ||
| get_layer_data(p, i = 2L) | ||
| ) | ||
|
|
||
| # name falls back to index | ||
| expect_snapshot_error( | ||
| get_layer_data(p, i ="none") | ||
| ) | ||
| expect_snapshot_error( | ||
| get_layer_data(p, i = 4L) | ||
| ) | ||
|
Comment on lines
+238
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is good that these errors are thrown, but I don't think ggplot2 needs to test for them. They are outside ggplot2's control as they are thrown by the vctrs package. If the message changes, that doesn't imply that ggplot2 needs to change. |
||
| }) | ||
|
|
||
| test_that("get_layer_grob works with layer names", { | ||
| p <- ggplot() + geom_point(name = "foo") + geom_point(name = "bar") | ||
|
|
||
| # name has higher precedence than index | ||
| expect_identical( | ||
| get_layer_grob(p, i = "bar"), | ||
| get_layer_grob(p, i = 2L) | ||
| ) | ||
|
|
||
| # name falls back to index | ||
| expect_snapshot_error( | ||
| get_layer_grob(p, i ="none") | ||
| ) | ||
| expect_snapshot_error( | ||
| get_layer_grob(p, i = 4L) | ||
| ) | ||
|
Comment on lines
+257
to
+262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise, I think we don't need these tests. |
||
| }) | ||
|
|
||
| test_that("data.frames and matrix aesthetics survive the build stage", { | ||
| df <- data_frame0( | ||
| x = 1:2, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intent of this test is good. However, I think the test would also pass if the index was mismatched because the layer data of both layers would be the same.