Skip to content

Conversation

@gordonwoodhull
Copy link
Collaborator

@gordonwoodhull gordonwoodhull commented Apr 28, 2025

Hi @cderv!

Here are helper functions applying colors or brands to five or so R plotting and table packages.

Companion to quarto-dev/quarto-python#9 for Python packages.

The corresponding adapted examples are on the quarto-helpers branch, helpers/ directory of my quarto-light-dark-experiments repo:

https://github.com/gordonwoodhull/quarto-light-dark-experiments/tree/use-quarto-helpers/helpers

devtools::check() does not seem to produce any extra warnings. Looks like we are seeing some warnings from earlier changes?

Undocumented arguments in documentation object 'quarto_publish_doc'
    ‘server’ ‘account’ ‘metadata’

I have tested locally with devtools::install()

Should I add any tests or vignettes?

naming convention

I am happy to change the naming convention. As a straw-man naming convention, I have adopted

{package}_theme_colors(bg, fg)

for a function that will return an appropriate object or function that applies the theme to that package.

Conventions are all over the board as to what is appropriate to return and how it will be used. (Theming is the most idiomatic thing you can do with any package. 😉 )

It might help to have examples, which we could e.g. adapt from quarto-light-dark-experiments.

{package}_theme_brand(brand_yml)

for a function that takes a brand.yml path and returns the same kind of appropriate object or function.

Again, I'm open to all design suggestions. Note that these aren't really Quarto functions: they are adaptors to help use other packages with Quarto.

FWIW the helper functions in quarto-python use the same straw-man naming convention. (Also subject to debate.)

@gordonwoodhull gordonwoodhull marked this pull request as draft April 28, 2025 20:07
@cderv cderv self-assigned this Apr 29, 2025
@cderv
Copy link
Collaborator

cderv commented Apr 29, 2025

Thanks gordon!

I must acknowledge this is going quick and I haven't thought much about those functions.

I believe it would be really cool to have vignette, or a pkgdown article to document using those functions on the website: https://quarto-dev.github.io/quarto-r/articles/

Having tests is also necessary, but if adding testthat tests is not easy for these, the article page on the pkgdown website could be a way to test them—pkgdown support .qmd and use quarto render. However, I don't know how it would work with the brand.

Do you think tests can be added using testthat ? Or we would need visual test ? We can have a dedicated page for testing in the pkgdown website, with some resources to download to check this is working. For example, the pkgdown package itself have some articles that are supposed to be only checked for tests: https://pkgdown.r-lib.org/articles/index.html#testing

Happy to discuss all this live if this is easier.

First thoughts feedback

About naming, I think would have gone with verb first, then specific package i.e. theme_brand_{package}

I guess I am used to dispatch of method, where this is theme_brand() generic, and then when applied on some object of class 'package', the method would dispatch to theme_brand.package()

Maybe we can't really use S3 method (or could we ?) - but it feels related instead of having one function per object.

But for auto completion, I think have {generic_fun_name}_{specifier} is better, as you can start with generic name and get the completion for the rest 🤷‍♂️ but it also works the other way to get all related function for a specific package {specifier}_{generic_fun_name} 😅 probably why those functions could live inside each of the package.

FWIW the helper functions in quarto-python use the same straw-man naming convention. (Also subject to debate.)

This looks like an important point to have same name in both packages.

To be discussed. Naming is hard!

@cderv cderv marked this pull request as ready for review April 29, 2025 09:45
@cderv cderv marked this pull request as draft April 29, 2025 09:45
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why you get a CI error. This should not be modified - this is probably happening because you did not installed all dev dependencies (Suggested package)

You can do so using pak::pak()

So this file should be reverted for the CI to pass

@gordonwoodhull
Copy link
Collaborator Author

gordonwoodhull commented Apr 29, 2025

Sure, the naming convention theme_colors_{package} and theme_brand_{package} seems fine to me.

Thanks for the tips. I have done a little R package development before, but using base R tools instead of pak and devtools and so on. Glad to learn the modern tools!

R is not liking the gfortran I have installed, but I'll figure it out.

Visual tests are easy for this. Automated tests may be difficult. If we are specifically testing that the theme_colors_* and theme_brand_* functions are working, we would need to look at the output of the packages, which is bitmap, html, svg.

The visual tests could serve as a smoke test that at least creating and using the themes using the latest API of the plotting / table packages doesn't crash. (All these packages are in Suggests: and intentionally unpinned.) Maybe that is sufficient?

@cderv
Copy link
Collaborator

cderv commented Apr 29, 2025

The visual tests could serve as a smoke test that at least creating the themes using the latest API of the plotting / table packages doesn't crash. (All these packages are in Suggests: and intentionally unpinned.) Maybe that is sufficient?

Yes definitely. I think this is sufficient. And if we want visual test using playwright for example, I would do them on quarto side using dev version of this package for example.

I am increasingly thinking of a dedicated repo for Quarto visual integration testing using Playwright so that everything does not run in Quarto-cli. However, until testing becomes a problem, I think we could add it there.

If we want to go visual testing mode for quarto-r, maybe a tweaked version of shinytest2 for quarto document (https://rstudio.github.io/shinytest2/index.html) would be necessary. But I think our Quarto-cli infra is enough.

@gordonwoodhull gordonwoodhull force-pushed the plot-theming branch 3 times, most recently from 4fd3634 to f9e8c6f Compare April 30, 2025 02:36
@gordonwoodhull
Copy link
Collaborator Author

I've updated the naming and got seven smoke tests (almost) working via testthat.

All they do is render the same examples I've copied from quarto-light-dark-experiments.

The remaining failure seems to be colorspace doing an illegal instruction trying to load its DLL on macOS.

I see that it's not possible to have light/dark switching in a vignette, because bootstrap is disabled. But could we have vignettes with custom CSS for background/foreground colors to show visually that theming of the plot is working?

I.e. on the Quarto HTML Vignettes page, when it says "A custom CSS file is provided." does that mean by the vignette system or by me?

I guess it would also be fine to just have vignettes that demonstrate use of background/foreground color in plots, but the vignette background is still white.

I'll squash this when it's closer to being ready - there was a lot of iteration over CI.

@cderv
Copy link
Collaborator

cderv commented Apr 30, 2025

But could we have vignettes with custom CSS for background/foreground colors to show visually that theming of the plot is working?

By default vignettes don't use bootstrap, but for our tests I think we can overwrite this and use as a vignette a "normal" quarto document. The vignette engine to use it
%\VignetteEngine{quarto::format} and a format key must be present in the document and will be used.

The quarto::html default vignette is an opinionated simpler one closer to what we expect for vignettes on CRAN. (like rmarkdown::html_vignette was doing instead of full rmarkdown::html_document)

I am missing documentation about that.

I.e. on the Quarto HTML Vignettes page, when it says "A custom CSS file is provided." does that mean by the vignette system or by me?

For the context, the way I implemented the default HTML vignette right now uses --metadata-file, and so it will have precedence over same configuration in YAML header.
Hence my addition in the doc:

All those configurations are set in way that they can’t be overriden by the YAML header in the vignette source file and only new configurations can be set.

However, I created (for advanced user and for us for example), the quarto::format I mentioned above, which will use the normal default for .qmd (so format: html if not provided, or the format: key)

Note that this is the vignette - for pkgdown's article, it uses the vignette .qmd, but the rendering is done by pkgdown itself, not the vignette engine

In their doc (https://pkgdown.r-lib.org/articles/quarto.html) I see in Limitations:

pkgdown assumes that you’re using quarto vignette style, or more generally an html format with minimal: true. Specifically, only HTML vignettes are currently supported.

So we're probably stuck... 😞 To be improved - I need to think about dark / light vignette 🤔

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing all the .Rd file, I am wondering if this would be better to document all the functions in a single R help page about dark-light feature. I think it would be best

@gordonwoodhull
Copy link
Collaborator Author

Seeing all the .Rd file, I am wondering if this would be better to document all the functions in a single R help page about dark-light feature. I think it would be best

Sure! These are pretty simple functions, and repetitive.

How do I do this? Looks like the rest of the functions are each documented on their own page.

Is this what roxygen's collate feature is for? I'm new to all this.

So we're probably stuck... 😞 To be improved - I need to think about dark / light vignette 🤔

I could do something which demonstrates the feature by showing a few plots using a few (or all) of the supporting packages, each with a different background and foreground color.

That would demonstrate the functionality exposed here, which doesn't really have to do with switching, just theming.

Not as cool, maybe, but functional?

@cderv
Copy link
Collaborator

cderv commented Apr 30, 2025

Is this what roxygen's collate feature is for? I'm new to all this.

Oh sorry - I should have linked to it. Yes this is roxygen feature. Mentioned here for example: https://r-pkgs.org/man.html#sec-man-multiple-functions (BTW great book with recent best practice on package development)

Detailed also in the roxygen2 doc: https://roxygen2.r-lib.org/articles/reuse.html#multiple-functions-in-the-same-topic

Not as cool, maybe, but functional?

Yes it seems functional. And probably the only thing we can do for now.

five sets of theme helpers for seven plotting and table packages
@gordonwoodhull
Copy link
Collaborator Author

gordonwoodhull commented May 1, 2025

Okay, we have a combined reference doc, and we have a vignette that demonstrates all seven packages with various background and foreground colors.

Aside from lingering test failures, I think this is ready to go?

@gordonwoodhull gordonwoodhull marked this pull request as ready for review May 1, 2025 22:03
not ready for prime-time yet
@cderv cderv merged commit 8943e79 into quarto-dev:main May 5, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants