Let ggplot handle histogran binning. Fix #198 #200
Conversation
|
This is still a work in progress, but the main idea is there... |
|
@tdhock any idea why the home page for this commit says "could not download .png", but the individual pages appear to be working? |
|
@tdhock @mkcor @chriddyp here are a few new tests that reflect these changes (do we want to match the default fill color?) http://ropensci.github.io/plotly-test-table/tables/b938fdab8311104f2dfe61cc4bf21798a0e48810/histogram-counts.html Here is the test for #198 -- I still have to fix and add a test for this one -- And the axis labels are off on this one for some odd reason -- |
|
the "could not download .png" happens when the plotly server returns an error instead of a png when building the test table. https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L390-L398 Usually you can just wait a while and re-build the test table when the server gets working again. I guess that is what you did, since I did not see any "could not download" errors on http://ropensci.github.io/plotly-test-table/tables/b938fdab8311104f2dfe61cc4bf21798a0e48810/index.html |
|
Looking good! I agree that this is the right approach, it'll be a much smaller request for very large data-sets, too. |
|
Looks great! 💃 by me, but @tdhock might want to do a more thorough code review |
|
I'll code review tomorrow. |
| @@ -242,6 +242,41 @@ gg2list <- function(p){ | |||
| if (!all(barmodes == barmodes[1])) | |||
| warning(paste0("You have multiple barcharts or histograms with different positions; ", | |||
| "Plotly's layout barmode will be '", layout$barmode, "'.")) | |||
There was a problem hiding this comment.
want to add a test with expect_warning?
There was a problem hiding this comment.
This warning existed before and I'm not sure of an example where it's applicable, but it might be worth keeping in case we run across an edge case...
|
other than that everything looks fine, including the visual tests. +1 |
Let ggplot handle histogran binning. Fix #198
This pull request started out as a fix for #198, but I quickly realized the implementation of
geom_bar()/geom_histogram()was suboptimal for a couple reasonsggplot(mtcars, aes(factor(vs)))+geom_bar()would throw a warning, but it shouldn’t.