search

Mikata-Project / ggthemr

Themes for ggplot2.
893 stars 108 forks source link

Prep for CRAN Submission #40

Closed amy17519 closed 4 years ago

amy17519 commented 4 years ago

@cttobin @FCACollin @daroczig please take a look!

We (@cttobin and @Mikata-Project) can discuss and announce further updates on ggthemr after this :)

daroczig commented 4 years ago

@amy17519 regarding writing the scales functions in the global env at https://github.com/cttobin/ggthemr/blob/0a31bb579b51b1ee4487f2957edf2227a63611d8/R/ggthemr.R#L56, ggplot2 3.3.2 might provide a more elegant solution:

Default continuous colour scales (i.e., the options() ggplot2.continuous.colour and ggplot2.continuous.fill, which inform the type argument of scale_fill_continuous() and scale_colour_continuous()) now accept a function, which allows more control over these default continuous_scale()s (@cpsievert, #3827).

Can you please look into that?

cttobin commented 4 years ago

These changes look good to me. It's nice to see a proper solution for the scales is available with options(). That is way better than what was there before.

One thing is I see you've corrected the spelling of "camouflage" which is obviously sensible. In the event that people already are using that theme maybe it would be safer to support both the original incorrect and corrected spellings so nobody has any broken code after upgrading. Potentially with a warning on the incorrect spelling. I'll leave that up to you to decide if it's work doing though, I wouldn't say it's critical.

I'm happy to merge (probably "Squash and merge" unless you prefer otherwise) if you're OK with that?

cttobin commented 4 years ago

Or maybe we should transfer ownership first and then you can merge however you like. I'll reply to the email you Gergely sent me anyway so we can sort it out there.