Da Big ojoThemes PR #2
Conversation
DESCRIPTION
Outdated
There was a problem hiding this comment.
Added ggplot2 and showtext to depends, this ensures the fonts can be handled by showtext and removes the need to run library(ggplot2) before attaching this package.
There was a problem hiding this comment.
This was actually wrong lol, they're all in imports now
| #' @family tok palettes | ||
| #' @rdname tok_palettes | ||
| #' @export | ||
| palette_tok_main <- c( |
There was a problem hiding this comment.
This kinda sucks and I just threw it together to have something in there. The whole TOK theme is kinda similar, I just wanted to get the bones in place
R/geoms.R
Outdated
|
|
||
| setup_data = function(data, params) { | ||
| data$width <- data$width %||% | ||
| params$width %||% (resolution(data$x, FALSE) * 0.7) # Set to 70% of resolution rather than default 90% |
There was a problem hiding this comment.
Literally the only difference from the default GeomCol ggproto is that this is 0.7 and not 0.9
| position = position, | ||
| show.legend = show.legend, | ||
| inherit.aes = inherit.aes, | ||
| params = list( |
There was a problem hiding this comment.
This is where we could change the default geom color, etc. too if we wanted.
| #' @param ... other arguments passed to \code{geom_line()} | ||
| #' @export | ||
| geom_line <- function(mapping = NULL, size = 1, ...) { | ||
| ggplot2::geom_line(mapping = mapping, size = size, ...) | ||
| geom_line <- function(mapping = NULL, linewidth = 1.5, ...) { |
There was a problem hiding this comment.
This doesn't require a whole new ggproto object because the linewidth doesn't cause the same data resolution issues as the col width.
| #' @param base_line_size The base line size to use; 0.5 is the default. | ||
| #' @param base_rect_size The base rect size to use; 0.5 is the default. | ||
| #' @export | ||
| theme_ojo <- function(base_size = 14, |
There was a problem hiding this comment.
I broke this out from theme_ojo_base() like we discussed @brancengregory
|
I'm not quite ready to think about merging this yet, but I've got a functional version of the 3 main themes in there (even though the TOK one looks like shit). I'll let y'all know when I'm ready for a final review! |
R/zzz.R
Outdated
There was a problem hiding this comment.
I don't think we need any of this in zzz.R, I did specify in the DESCRIPTION that we need version 3.0.0 of ggplot2 though
There was a problem hiding this comment.
I could be wrong though, I'm not 100% sure what it was doing
…ct due to deprecation; add basic vdiffr snapshot tests
|
This is all ready for review now @anthonyokc |
| } | ||
|
|
||
| # add base_family font to text and label geoms --------------------------- | ||
| ggplot2::update_geom_defaults("text", list(family = base_family)) | ||
| ggplot2::update_geom_defaults("label", list(family = base_family)) | ||
| ggplot2::update_geom_defaults("text_repel", list(family = base_family)) | ||
| ggplot2::update_geom_defaults("label_repel", list(family = base_family)) | ||
| # ggplot2::update_geom_defaults("text_repel", list(family = base_family)) # Keep ggrepel in this? |
There was a problem hiding this comment.
Is there any reason not to keep it? Does it require us having ggrepel installed or anything? If not I'd keep it.
There was a problem hiding this comment.
I don't think it should require ggrepel if I'm understanding correctly, this should just do nothing if it's not installed I think.
|
Made some changes to the spacing and gridlines for the themes
I'd still like to fix the spacing between the legend items (see how the text for a legend item is too close to the box to the right of it), so might get to that quickly tomorrow (or someone else can if they have the time, you'd probably just mess with the legend.key.spacing.x). |
|
I think I fixed your last point about horizontal spacing on the legend @anthonyokc |
I accidentally committed a few things to main at first, but this PR will track my rewrites / implementations of
theme_ojo(),theme_okpi(), and hopefully alsotheme_tok().