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

Sort ribbon/area in setup_data #3023

Merged
merged 3 commits into from Dec 4, 2018

Conversation

Projects
None yet
2 participants
@thomasp85
Member

thomasp85 commented Dec 4, 2018

This PR streamlines the handling of data in line-like gloms where an implicit sorting of data along the x-axis is happening. Previously GeomLine had the sorting happening in setup_data while GeomRibbon (and by extension GeomArea) had it inside draw_group.

The motivation is that it solves some issues in gganimate that couldn't be fixed outside of ggplot2, but I also think that conceptually this is the right approach, and we should be consistent in how we treat data operations across kindred geoms/stats

@thomasp85 thomasp85 requested a review from clauswilke Dec 4, 2018

@clauswilke

Looks good to me. It might cause some downstream effects though for any third-party geoms that inherit from GeomArea and expect the old behavior, so maybe make a note of this change in NEWS.md.

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Dec 4, 2018

Yeah, maybe... I think in the worst case they just do an additional sort on an already sorted data.frame, but who knows... I'll add a note

thomasp85 added some commits Dec 4, 2018

@thomasp85 thomasp85 merged commit a8e9668 into tidyverse:master Dec 4, 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
@clauswilke

This comment has been minimized.

Member

clauswilke commented Dec 4, 2018

Actually, if they implement their own setup_data() function but use the draw_group() function provided by GeomRibbon then they'll draw unsorted data. Just like you had to fix setup_data() for GeomArea.

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Dec 4, 2018

That is true. The note is there anyway, but I don’t expect any problems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment