-
Notifications
You must be signed in to change notification settings - Fork 26
theme helper functions #234
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
Conversation
|
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 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 feedbackAbout naming, I think would have gone with verb first, then specific package i.e. I guess I am used to dispatch of method, where this is 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
This looks like an important point to have same name in both packages. To be discussed. Naming is hard! |
There was a problem hiding this comment.
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
|
Sure, the naming convention Thanks for the tips. I have done a little R package development before, but using base R tools instead of R is not liking the Visual tests are easy for this. Automated tests may be difficult. If we are specifically testing that the 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 |
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. |
4fd3634 to
f9e8c6f
Compare
|
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 The remaining failure seems to be 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. |
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 The I am missing documentation about that.
For the context, the way I implemented the default HTML vignette right now uses
However, I created (for advanced user and for us for example), the 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:
So we're probably stuck... 😞 To be improved - I need to think about dark / light vignette 🤔 |
There was a problem hiding this 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
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
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? |
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
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
242452e to
0d621ae
Compare
|
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? |
not ready for prime-time yet
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-helpersbranch,helpers/directory of myquarto-light-dark-experimentsrepo: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?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
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.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.)