Skip to content

Add tests for values, limits, and breaks in manual scale #4547

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

Conversation

banfai
Copy link
Contributor

@banfai banfai commented Jul 7, 2021

Issue introduced in #4471, if there are extra named values in a manual scale then they will show up on the legend even with drop = TRUE.

Question: should these values be forced with drop = FALSE? (currently it's possible)

* issue introduced in tidyverse#4471
* potential solution for tidyverse#4534 and tidyverse#4511
* add extra tests
@msberends
Copy link

Question: should these values be forced with drop = FALSE? (currently it's possible)

I would very much like so. Probably a nice option to add legend items not present in the data if the plot is part of a series, or e.g. to make it obvious that a factor contains levels that have zero observations.

@yutannihilation
Copy link
Member

@banfai
Hi, thank you for creating this pull request. Recently, we got an alternative pull request (#4619) that tackles the same issue. We are getting to feel #4619 is preferred in that the change is only on manual scale, while your pull request modifies the whole Scale.

Would you mind tweaking this pull request to contain only the tests? These are helpful to check if #4619 does the right thing, and, what's more important, I'd like to make it clear that you are a contributor by merging this. The CI checks will of course fail, but it's fine. If you don't have time, I can do it.

@banfai
Copy link
Contributor Author

banfai commented Sep 18, 2021

Hi @yutannihilation @teunbrand

Thanks for having a look at this. I have seen @teunbrand 's comment earlier on 4511 and agreed that it was a leaner solution, but haven't had time to look deeper or change this PR.
I have removed the changes in this and added some more tests. These pass on my computer with #4619 included.

@banfai banfai changed the title Remove extraneous limit values in manual scale Add tests for values, limits, and breaks in manual scale Sep 18, 2021
@yutannihilation yutannihilation merged commit daed593 into tidyverse:master Sep 19, 2021
@yutannihilation
Copy link
Member

Thank you!

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