-
Notifications
You must be signed in to change notification settings - Fork 633
Updates to better support dendrograms #818
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: master
Are you sure you want to change the base?
Conversation
R/layers2traces.R
Outdated
grepl("italic", data[["fontface"]]), | ||
paste0("<i>", text, "</i>"), | ||
text | ||
) |
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'd prefer if this was done with if()
instead of ifelse()
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 was using ifelse
as I was anticipating data[["fontface"]]
could be a vector (I think it can be specified via aes
). Is this an incorrect assumption?
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.
oops, yea, but in that case I prefer something like:
text[grepl("italic", data[["fontface"]])] <- paste0("<i>", text, "</i>")
cbind(x = ifelse(xmax == Inf, layout$x_max, xmax), | ||
y = ifelse(ymin == -Inf, layout$y_min, ymin), others)) | ||
}) | ||
} |
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.
This is a lot of redundant code...
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.
Good point, I've simplified things a bit.
R/ggplotly.R
Outdated
@@ -702,6 +702,7 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", | |||
) | |||
|
|||
# if theme(legend.position = "none") is used, don't show a legend _or_ guide | |||
npscales$scales <- Filter(function(x) x$guide != "none", npscales$scales) | |||
if (npscales$n() == 0 || identical(theme$legend.position, "none")) { |
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 see this as potentially dangerous -- a scale without a guide doesn't necessarily mean it doesn't exist.
I would prefer to instead set layout.showlegend
to FALSE
for discrete scales and remove the colorbar for continuous ones.
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.
How about instead handling the discrete scale by updating this line to something like
sc$is_discrete() && sc$guide != "none"
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.
That won't work either for similar reasons
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 made one more attempt, hopefully we're close. Thanks.
Sorry for the delay @jrowen, this is looking pretty good...before I merge, would you mind writing a couple simple tests? |
Sorry, I too have been away from this for a while. I'll see if I can pull those together shortly. Thanks. |
Hi @jrowen Thanks! |
I added some checks for
|
Don't worry about the build failure @jrowen. I'll have a closer look here next week |
Hi Carson, Thanks, |
I may get to this next week. #929 will take precedence. |
textposition = paste0( | ||
ifelse(data[["vjust"]] < 0.5, "top ", | ||
ifelse(data[["vjust"]] > 0.5, "bottom ", "") | ||
), |
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.
Shouldn't the trailing whitespace be removed here (e.g., "top"
not "top "
)?
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.
Ahh, nevermind. I see why it's done this why
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 this should default to "middle"
(not ""
) though
cbind(x = ifelse(xmax == Inf, x_max, xmax), | ||
y = ifelse(ymax == Inf, y_max, ymax), others), | ||
cbind(x = ifelse(xmax == Inf, x_max, xmax), | ||
y = ifelse(ymin == -Inf, y_min, ymin), others)) | ||
}) |
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.
Perhaps it's better (more efficient) to use is.finite()
here?
@@ -139,3 +139,36 @@ test_that('Specifying alpha in hex color code works', { | |||
expect_match(info$data[[1]]$fillcolor, "rgba\\(0,0,0,0\\.0[6]+") | |||
}) | |||
|
|||
p1 = ggplot(data.frame(x = 1, y = 1)) + |
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.
It'd be great to have another test that specifying ymin = 0.5
/ymax = 1.5
gives the same result
Hey @cpsievert :) |
These changes were made to better support dendrograms, particularly those created with
dendextend
. It adds support forfontface
ingeom_text
hjust
andvjust
ingeom_text
-Inf
andInf
ingeom_rect
guide = "none"
Feedback is very welcome. Below is some sample code.