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

Various fixes to themes #1679

Merged
merged 11 commits into from Aug 23, 2016

Conversation

Projects
None yet
4 participants
@jiho
Contributor

jiho commented Jul 25, 2016

Make both the code and the visual aspect of all theme more homogeneous. The general idea was to take theme_grey as the reference for all changes (and since it changed slightly in the previous update of ggplot2, the other themes needed some love too!).

I tried to divide the PR in small standalone commits (too lazy for several pull requests ;-) ). I'll update them as needed so that the show PR can be merged. I also added the rationale for various choices as comments in the code.

Some fixes were clearly needed (default size, etc.). Visually, I think theme_bw in particular needed a review. For the others, the changes are quite subtle.

@jiho

This comment has been minimized.

Contributor

jiho commented Jul 25, 2016

Some before-after shots; current stable on the left, new version after these commits on the right
compare002
compare003
compare004
compare005
compare006
compare007
compare008
compare009

@hadley

This comment has been minimized.

Member

hadley commented Jul 28, 2016

Does this connect to #1581 in any way?

I never really use themes, so I'd also be open to suggestions about how to decrease the likelihood of regressions when I merge future PRs.

@hadley hadley added the ready label Jul 28, 2016

@jiho jiho force-pushed the jiho:fix/themes branch from 89c2cf4 to be20b8b Jul 29, 2016

@jiho

This comment has been minimized.

Contributor

jiho commented Jul 29, 2016

I don't think it relates to #1581 in any way. I think the point of that PR is well made. Once it is merged, it would (marginally) simplify some of the theme specifications but it should not create any regression.

As for regressions for the changes in this PR, it depends what you would call a regression. Some commits here do change the visual aspect. I think for the better but some might say any change is a regression at this point. You have the before-after shots to judge which are acceptable. I think the most problematic, potentially, would be theme_bw, which is the probably the most used and the one I changed the most. I think the previous version had serious issues (colors, minor grid larger than major grid, etc.).

PS: The merge tool says there are conflicts but I just branched from master a few days ago. I've changed the commits to solve a Travis error; do do so I've had to force push the new commits. Maybe this throws the merge tool astray. Let me know if you want to rebase.

@hadley

This comment has been minimized.

Member

hadley commented Aug 5, 2016

Ok, sounds reasonable. If you rebase, I'll merge.

@Katiedaisey

This comment has been minimized.

Contributor

Katiedaisey commented Aug 5, 2016

I'll check more closely later with some edge cases but I see no obvious conflict with #1581. This seems to standardize the defaults while #1581 incorporates values from more general parent elements when intermediate elements are not specified.

jiho added some commits Jul 25, 2016

Match theme_bw with theme_grey better
Keep contrast and size of gridlines (in particular, previously the minor
grid lines were thicker than the major ones which looked weird)

Remove the border around the legend key which complicates things visually
for little gain

Outline panel and facet strips with a dark-ish grey
Simplify theme_linedraw
Use strip.text instead and strip.text.x and strip.text.y separately

Slightly thicker grid lines compared to the previous iteration (but still
quite thin)

Base on theme_bw rather than on theme_grey
Improve and homogenise theme_dark and theme_light
Use strip.text instead and strip.text.x and strip.text.y separately

As in theme_grey and theme_bw, keep the same color for major and minor
gridlines; just change the thickness (as was already done before).

In theme_dark, make the strips the inverse of what they are in theme_grey
Reorder elements in the specification of theme_minimal
Just to match theme_grey. No visual change beyond the changes to theme_bw
on which this is based
Fix regressions in theme_classic
Explicitly add axes which disappeared due to new specifications in
theme_bw at some point.

Increase size of the borders of strips to match axes visually.
Keep legend title in theme_void
(otherwise the plot may be un-understandable)
Move theme_dark next to theme_light in the code
They are related and their examples are in this order

@jiho jiho force-pushed the jiho:fix/themes branch from be20b8b to 1c25dc9 Aug 23, 2016

@jiho

This comment has been minimized.

Contributor

jiho commented Aug 23, 2016

I've rebased. I'm not sure what that failing check means...

@hadley hadley merged commit dc99631 into tidyverse:master Aug 23, 2016

2 of 3 checks passed

codecov/patch 13.95% of diff hit (target 67.31%)
Details
codecov/project 67.71% (+0.40%) compared to 0da7d47
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hadley hadley removed the ready label Aug 23, 2016

@hadley

This comment has been minimized.

Member

hadley commented Aug 23, 2016

Thanks! The failing check is just for unit tests, which don't matter here

@jiho

This comment has been minimized.

Contributor

jiho commented Aug 23, 2016

That was quick, thanks. Do you want me to test how #1728 behaves after this merge?

@jiho jiho deleted the jiho:fix/themes branch Aug 23, 2016

@hadley

This comment has been minimized.

Member

hadley commented Aug 23, 2016

I'll update that branch, but I think it should be largely independent.

mnbram added a commit to mnbram/ggplot2 that referenced this pull request Aug 26, 2016

Various fixes to themes (tidyverse#1679)
* Match default font size to new default

Set in theme_grey to 11

* Match theme_bw with theme_grey better

Keep contrast and size of gridlines (in particular, previously the minor
grid lines were thicker than the major ones which looked weird)

Remove the border around the legend key which complicates things visually
for little gain

Outline panel and facet strips with a dark-ish grey

* Simplify theme_linedraw

Use strip.text instead and strip.text.x and strip.text.y separately

Slightly thicker grid lines compared to the previous iteration (but still
quite thin)

Base on theme_bw rather than on theme_grey

* Improve and homogenise theme_dark and theme_light

Use strip.text instead and strip.text.x and strip.text.y separately

As in theme_grey and theme_bw, keep the same color for major and minor
gridlines; just change the thickness (as was already done before).

In theme_dark, make the strips the inverse of what they are in theme_grey

* Reorder elements in the specification of theme_minimal

Just to match theme_grey. No visual change beyond the changes to theme_bw
on which this is based

* Fix regressions in theme_classic

Explicitly add axes which disappeared due to new specifications in
theme_bw at some point.

Increase size of the borders of strips to match axes visually.

* Keep legend title in theme_void

(otherwise the plot may be un-understandable)

* Improve comments and reorder elements to match other theme specification

* Fix alignment of code in all theme functions

* Add NEWs bullet for theme update

* Move theme_dark next to theme_light in the code

They are related and their examples are in this order
@krlmlr

This comment has been minimized.

Member

krlmlr commented Apr 14, 2017

Is there a convenient way to access the old definitions? I need to retrofit a theme_bw() plot to the old look, my current workaround is to install ggplot2 2.1.0 :-/

@jiho

This comment has been minimized.

Contributor

jiho commented Apr 14, 2017

You can probably pull the file with the definitions from 2.1.0 and override the new ones by sourcing just this file... unless this was before the theming system changed.

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