ddsjoberg / gtsummary

Presentation-Ready Data Summary and Analytic Result Tables
http://www.danieldsjoberg.com/gtsummary
Other
1.03k stars 113 forks source link

Thoughts/Updates for {gtsummary} themes #1580

Open ddsjoberg opened 8 months ago

ddsjoberg commented 8 months ago

@larmarange and I recently had a chat about gtsummary themes. @larmarange indicated some students have confusion with gtsummary themes because they are specified differently than ggplot2 themes.

Admittedly, I do prefer the ggplot2 specification using ggplot() + geom_line() + themes(). But I am not sure how I could make this work in gtsummary. Essentially we would need to delay the calculation of everything until the print (or as_gt(), as_kable() etc) is called. That would require much more complex environment handling and a major re-write. While not impossible, I dont know if that added complexity is worth trying to unify ggplot and gtsummary notation. I'll keep thinking on it...maybe it will be worth it!

There was another thing I was thinking about regarding themes' current implementation that I do not love: If I set a theme at the top of a script and create a gtsummary table and save out the object, the theme is only active for the parts that are used during the construction of the table. If I were to load the saved gtsummary table in another script, and print it, any theme elements that control the defaults in the as_gt() function would not be activated unless I were to load the same theme into the env before printing. This is something that has confused users. Perhaps, we could instead save the "theme env" with the gtsummary object when the tbl_*() functions are called and the themes would be available for that tbl_*() object in any environment.

larmarange commented 8 months ago

If I may, some additional thoughts.

If the "theme env" is saved with the tbl object, you could consider two different options: set_gtsummary_theme() to change overall the theme elements for the future gtsummary objects that will be created, and a modify_gtsummary_theme() that could be applied on an existing table, e.g. tbl %>% modify_gtsummary_theme().

Another thought is the question of inheritance of theme elements. It exists in the code in many places, without being explicit. An example:

https://github.com/ddsjoberg/gtsummary/blob/30ef7175b9cc935ddcfd41f7c9c2418f3ee52cd8/R/tbl_svysummary.R#L153-L157

We could think about a better naming of the theme elements, showing the inheritance. An example:

Then the code could be simplify in many places. We could simply call get_theme_element("pvalue_fun.summary.custom"). If this element is not defined (i.e. NULL) then get_theme_element() will look at "pvalue_fun.summary" and eventually at "pvalue_fun". With such naming (also used by ggplot2), inheritance will be explicit.

Another thought coming from ggplot::theme(): each theme element should be a named argument of set_gtsummary_theme() and modify_gtsummary_theme(). It would allow to use autocompletion to easily identify the name of a theme element, and improve the documentation of all of them.

larmarange commented 8 months ago

Also, an internal function called when package is loaded to fix the default values at once would be relevant, to avoid indicating default values within each function.