Skip to content

Fix bar chart conversion for position="dodge" (barmode="group" in Plotly) #129

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

Merged
merged 7 commits into from
Oct 21, 2014

Conversation

mkcor
Copy link
Contributor

@mkcor mkcor commented Oct 20, 2014

Also including fairly unrelated commits in here:

  • removing image files from master (they will live on a branch dedicated to the image diff framework);
  • style in image-diff--related files (will move to above-mentioned branch);
  • updating NEWS file.

This PR offers a working conversion for ggplot2 plots made with geom_bar() and position_dodge(): before (https://plot.ly/~ggplot2-cookbook/16) vs after (https://plot.ly/~marianne2/80). Strengthened related test accordingly.

/cc @pedrodz @chriddyp

@chriddyp
Copy link
Member

Nice! where can we check out the image diffs now?

@mkcor
Copy link
Contributor Author

mkcor commented Oct 20, 2014

It's coming, it's coming...
As you can see, a test is failing, my workaround didn't catch this case, where

g$data
  PANEL    fill       x  y fill.name  x.name group ymin ymax xmin xmax
2     1 #F8766D    <NA> 14       Bio  Canada     1    0   14 0.55 1.00
1     1 #00BFC4    <NA> 23      Math  Canada     2    0   23 1.00 1.45
3     1 #F8766D Germany 37       Bio Germany     3    0   37 1.55 2.45
4     1 #00BFC4     USA 20      Math     USA     4    0   20 2.55 3.45

not all x values are NA...
Let me fix this first.

@mkcor
Copy link
Contributor Author

mkcor commented Oct 20, 2014

@chriddyp Image diffs can be seen over here: https://github.com/ropensci/plotly/compare/add-r-cookbook-tests 💃
Ctrl-F for "bar-dodge" at fe2a484

@mkcor
Copy link
Contributor Author

mkcor commented Oct 20, 2014

Before: https://plot.ly/~marianne2/85, after: https://plot.ly/~marianne2/86
Rerunning tests on branch add-r-cookbook-tests now...

@mkcor
Copy link
Contributor Author

mkcor commented Oct 20, 2014

This last commit does not affect 4ca177b
This should be ready for review now :)
Thanks, @chriddyp
We can reorganize/improve the add-r-cookbook-tests branch incrementally, and as we deem appropriate.

@pedrodz
Copy link
Contributor

pedrodz commented Oct 21, 2014

Tested... Looks great. Thanks @mkcor!

mkcor added a commit that referenced this pull request Oct 21, 2014
Fix bar chart conversion for position="dodge" (barmode="group" in Plotly)
@mkcor mkcor merged commit c47df47 into master Oct 21, 2014
@mkcor mkcor deleted the marianne-fix-bar-null branch October 21, 2014 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants