Skip to content
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

Order labels in facet_wrap with drlib::reorder_within #110

Merged
merged 2 commits into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@tmastny
Copy link
Contributor

commented Mar 24, 2018

In the tidytext book, graph labels often need to be reordered by the count. In fact, this struggle has been documented in a few blog posts:

Including ggplot issue #1902 and pull request #1917.

Currently, Hadley suggests David Robinson's solution found in his personal library.

I thought it might be useful to include this function within the tidytext package, since it is a common operation within the tidytext book, and it might be easier to find this function if it was in this package.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 24, 2018

Codecov Report

Merging #110 into master will decrease coverage by 1.32%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage   82.19%   80.86%   -1.33%     
==========================================
  Files          11       12       +1     
  Lines         365      371       +6     
==========================================
  Hits          300      300              
- Misses         65       71       +6
Impacted Files Coverage Δ
R/reorder_within.R 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a694a6a...d33e389. Read the comment docs.

@juliasilge

This comment has been minimized.

Copy link
Owner

commented Mar 24, 2018

Thanks for your interest in this project!

Perhaps Dave and I can chat about this again, but we had previously said that tidytext is not a good place for these functions to live, since they have more general applications, are plotting functions, etc. Any new thoughts on this, @dgrtwo?

@juliasilge

This comment has been minimized.

Copy link
Owner

commented Dec 20, 2018

After pondering integrating this function for (checks calendar) NINE MONTHS (my fault here) I am going to merge this PR and officially keep/support this function in tidytext. It's a bit out there compared to what else we have in the package, but on the whole, I'd rather have it somewhere easy for users to access (i.e. a CRAN package) than nowhere.

@juliasilge juliasilge merged commit 84a9f95 into juliasilge:master Dec 20, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@juliasilge

This comment has been minimized.

Copy link
Owner

commented Dec 20, 2018

Thanks so much for your suggestion here, and for your patience! 🙌

juliasilge added a commit that referenced this pull request Dec 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.