Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Updates to better support dendrograms #818

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a6663c4
Improvements for dendrograms
jrowen Nov 30, 2016
208eed4
Improvements for dendrograms
jrowen Nov 30, 2016
9489610
Merge branch 'master' of https://github.com/jrowen/plotly
jrowen Nov 30, 2016
4dcdebb
Fix rect to handle coord flip
jrowen Nov 30, 2016
10936b4
Fix fontface
jrowen Nov 30, 2016
f7cce29
Fix fontface
jrowen Nov 30, 2016
7862f00
Fixed rect
jrowen Nov 30, 2016
3e63f3c
Cleanup hjust
jrowen Nov 30, 2016
16d097b
Clean-up GeomRect
jrowen Nov 30, 2016
dac04d2
Update fontface approach
jrowen Nov 30, 2016
180fcfd
Updated approach for guide = none
jrowen Nov 30, 2016
183b39f
Another guide = none attempt
jrowen Nov 30, 2016
88110c0
Added geom_text and geom_rect tests
jrowen Apr 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions R/ggplotly.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")) {
Copy link
Collaborator

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.

Copy link
Author

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"

Copy link
Collaborator

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

Copy link
Author

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.

gglayout$showlegend <- FALSE
} else {
Expand Down
44 changes: 43 additions & 1 deletion R/layers2traces.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,29 @@ to_basic.GeomRect <- function(data, prestats_data, layout, params, p, ...) {
cbind(x = xmax, y = ymax, others),
cbind(x = xmax, y = ymin, others))
})
if (inherits(p$coordinates, "CoordFlip")) {
dat <- with(data, {
rbind(cbind(x = ifelse(xmin == -Inf, layout$y_min, xmin),
y = ifelse(ymin == -Inf, layout$x_min, ymin), others),
cbind(x = ifelse(xmin == -Inf, layout$y_min, xmin),
y = ifelse(ymax == Inf, layout$x_max, ymax), others),
cbind(x = ifelse(xmax == Inf, layout$y_max, xmax),
y = ifelse(ymax == Inf, layout$x_max, ymax), others),
cbind(x = ifelse(xmax == Inf, layout$y_max, xmax),
y = ifelse(ymin == -Inf, layout$x_min, ymin), others))
})
} else {
dat <- with(data, {
rbind(cbind(x = ifelse(xmin == -Inf, layout$x_min, xmin),
y = ifelse(ymin == -Inf, layout$y_min, ymin), others),
cbind(x = ifelse(xmin == -Inf, layout$x_min, xmin),
y = ifelse(ymax == Inf, layout$y_max, ymax), others),
cbind(x = ifelse(xmax == Inf, layout$x_max, xmax),
y = ifelse(ymax == Inf, layout$y_max, ymax), others),
cbind(x = ifelse(xmax == Inf, layout$x_max, xmax),
y = ifelse(ymin == -Inf, layout$y_min, ymin), others))
})
}
Copy link
Collaborator

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...

Copy link
Author

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.

prefix_class(dat, c("GeomPolygon", "GeomRect"))
}

Expand Down Expand Up @@ -603,10 +626,21 @@ geom2trace.GeomBoxplot <- function(data, params, p) {

#' @export
geom2trace.GeomText <- function(data, params, p) {
text <- as.character(data[["label"]])
text <- ifelse(
grepl("bold", data[["fontface"]]),
paste0("<b>", text, "</b>"),
text
)
text <- ifelse(
grepl("italic", data[["fontface"]]),
paste0("<i>", text, "</i>"),
text
)
Copy link
Collaborator

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()

Copy link
Author

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?

Copy link
Collaborator

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>")

compact(list(
x = data[["x"]],
y = data[["y"]],
text = data[["label"]],
text = text,
key = data[["key"]],
frame = data[["frame"]],
ids = data[["ids"]],
Expand All @@ -618,6 +652,14 @@ geom2trace.GeomText <- function(data, params, p) {
aes2plotly(data, params, "alpha")
)
),
textposition = paste0(
ifelse(data[["vjust"]] < 0.5, "top ",
ifelse(data[["vjust"]] > 0.5, "bottom ", "")
),
Copy link
Collaborator

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 ")?

Copy link
Collaborator

@cpsievert cpsievert May 29, 2018 •
edited
Loading

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

Copy link
Collaborator

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

ifelse(data[["hjust"]] < 0.5, "right",
ifelse(data[["vjust"]] > 0.5, "left", "center")
)
),
type = "scatter",
mode = "text",
hoveron = hover_on(data)
Expand Down