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

Various fixes to themes #1679

Merged
merged 11 commits into from
Aug 23, 2016
Merged

Various fixes to themes #1679

merged 11 commits into from
Aug 23, 2016

Conversation

jiho
Copy link
Contributor

@jiho 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
Copy link
Contributor Author

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
Copy link
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.

@jiho
Copy link
Contributor Author

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
Copy link
Member

hadley commented Aug 5, 2016

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

@Katiedaisey
Copy link
Contributor

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 11 commits August 23, 2016 18:01
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
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
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
Just to match theme_grey. No visual change beyond the changes to theme_bw
on which this is based
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.
(otherwise the plot may be un-understandable)
They are related and their examples are in this order
@jiho
Copy link
Contributor Author

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
@hadley hadley removed the ready label Aug 23, 2016
@hadley
Copy link
Member

hadley commented Aug 23, 2016

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

@jiho
Copy link
Contributor Author

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 fix/themes branch August 23, 2016 16:17
@hadley
Copy link
Member

hadley commented Aug 23, 2016

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

mnbram pushed a commit to mnbram/ggplot2 that referenced this pull request Aug 26, 2016
* 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
Copy link
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
Copy link
Contributor Author

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.

@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.

4 participants