support for several axis calls on same side #410
Conversation
| # remove | ||
| if (length(which.axis) > 1){ | ||
| # remove all axis functions other than the first one | ||
| object[[as.side_name(side)]] <- object[[as.side_name(side)]][-which.axis[!which.axis %in% head(which.axis, 1)]] |
There was a problem hiding this comment.
I was thinking you'd want to remove everything but the last one
There was a problem hiding this comment.
hmm...we should discuss the expected behavior for this.
There was a problem hiding this comment.
@ldecicco-USGS and @jiwalker-usgs do you have thoughts on this?
There was a problem hiding this comment.
My reasoning was this:
using the append arg to get multiple axis calls in there means appending them to the original call, which is the default set-up when the side is created. When you then set append = FALSE (what it is checking for here), those appended axis calls are wiped out.
I can see your point too, where you would want to treat it like append was FALSE the whole time and use the last, but that isn't the same behavior either.
For example:
gs <- gsplot() %>%
points(0:1,0:1) %>%
axis(side=1, at=c(0.5,1), labels=FALSE) %>%
axis(side=1, at=c(0.25, 0.75), line = 2, append=TRUE) %>%
axis(side=1, at=c(0.45, 0.55), lwd = 2, append=TRUE) %>%
axis(side=1, at=c(0.33))the last call uses append = FALSE, so what does that mean? Ignore all of the other TRUE calls and treat them as FALSE (collapsing all of their arguments into one), or does it mean remove all appended axis calls and build off of the default (what it currently does)?
There was a problem hiding this comment.
I would think in that example the last axis call replaces everything above it since it does not have append.
gs <- gsplot() %>%
points(0:1, 0:1) %>%
axis(side=1, at=c(0.5, 1)) %>%
axis(side=1, at=c(0.25, 0.75), line = 2, append=TRUE) %>%
axis(side=1, at=c(0.45, 0.55), lwd = 2, append=TRUE) %>%
axis(side=1, at=c(0.33))
gsCreates
There was a problem hiding this comment.
yes, that is what happens now with this PR
There was a problem hiding this comment.
hmmm I guess your comment says " remove all axis functions other than the first one" and that makes me think that everything except axis(side=1, at=c(0.5, 1)) is removed. Nevermind if it's removing all except axis(side=1, at=c(0.33))!
| which.axis <- which(names(object[[side.name]]) == 'axis') | ||
| if (length(which.axis) > 1){ | ||
| for (axis.i in which.axis){ | ||
| tmp <- object |
There was a problem hiding this comment.
what's the purpose of creating tmp?
There was a problem hiding this comment.
this was kind of a hack to deal with multiple axis function calls in one object. tmp is the same object, but w/o the other axis calls, that way I can pass tmp back into the same function and it will be treated the same way a single axis object is. Make sense? This function in general needs some attention, and I just added some new junk to it.
There was a problem hiding this comment.
Hmmm I think I understand, but isn't tmp changing each time from the new axis on the next line?
There was a problem hiding this comment.
the next line just has it remove all of the other axis calls in tmp. This loops through the axis calls and tmp is re-created each time based on the original object.
There was a problem hiding this comment.
the object[[as.side_name(side)]][['axis']] <- append_replace(object[[as.side_name(side)]][['axis']], user_args[[fun.name]]) append_replace makes sure the new arguments override the existing ones.
| axis(side=1, at=c(0.25, 0.75), append=TRUE) %>% | ||
| axis(side=1, at=c(0.45, 0.55), append=TRUE) %>% | ||
| axis(side=1, at=c(0.33)) | ||
| expect_equal(sum(names(gs$side.1) == 'axis'), 1) |
There was a problem hiding this comment.
any way to add a test for making sure it's keeping the expected axis and dropping the others?
There was a problem hiding this comment.
good idea @lindsaycarr I will add that
There was a problem hiding this comment.
@lindsaycarr can you merge if you think this is good to go?
|
fixing #408 |
No description provided.