Skip to content

Make sure all themes have visual tests #2174

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 3 commits into from
Jun 20, 2017
Merged

Conversation

karawoo
Copy link
Member

@karawoo karawoo commented Jun 19, 2017

Adds visual tests for theme_linedraw() and theme_test().

@karawoo karawoo requested a review from hadley June 19, 2017 22:29
@hadley
Copy link
Member

hadley commented Jun 19, 2017

You don't need to test theme_test() (and in fact you definitely shouldn't test it), because it's the theme used for most tests and should never change (if it does will have much breakage in the tests).

Per @baptiste's suggestion, can you please also test all with base_size = 33 (under the assumption if it looks good at 100% and 300% it's probably good in between)

@karawoo
Copy link
Member Author

karawoo commented Jun 19, 2017

Ah ok, I reverted the theme_test() test.

I'm a little confused about what the new tests should be testing. As far as I can tell the base_size argument only affects text elements (and a few margins). The changes in 2173 don't alter that, so I'm not sure if this will address @baptiste's concerns about the line widths. Should line/rect elements scale with base_size as well?

@hadley
Copy link
Member

hadley commented Jun 19, 2017

Oh good point. Maybe we should have a base line size too?

@karawoo
Copy link
Member Author

karawoo commented Jun 20, 2017

I can add that if you want. Would this be additional argument to the theme functions, or would it be better to set the base line size within the theme according to the base font size, and then let people override that with + theme(line = element_line(size = 1)) or whatever? Either way this seems like it deserves its own PR. I think the AppVeyor build failed on this one for reasons unrelated to the changes I made, but I don't see a way to force start a new build the way I can with Travis...

@hadley
Copy link
Member

hadley commented Jun 20, 2017

I think it would work in the same way as base_size - i.e. line = element_line(size = base_line_size). I guess it would default to base_size / 22? I agree that this should be a different PR, and you can go ahead and merge this one.

@lock
Copy link

lock bot commented Jan 18, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants