Skip to content
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

Add shadow_clone() to use themes #233

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tcgriffith
Copy link
Collaborator

@tcgriffith tcgriffith commented Oct 31, 2019

To contribute to this repo, please make sure to:

  • Sign the CLA http://www.clahub.com/agreements/yihui/xaringan (if you haven't signed it before).
  • Add a news item to NEWS.md and your name to DESCRIPTION (by alphabetical order) unless you have already been listed there.
  • Provide a URL to a live example (and even better, a couple of screenshots) if you are contributing a new theme.

Thank you!


The proposed function shadow_clone() is a product from discussions in #232

In short, we propose to separate the xaringan themes from the main repo, so that users can host their own xaringan themes in a separate repo.

Summary:

  • The shadow_clone() function works with existing built-in themes and remotely-hosted themes
  • For remote themes, git hash is resolved with a message addressing how to reproduce the exact theme.

Live example
xaringan source

Copy link
Collaborator

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

Thanks for starting on this one @tcgriffith!

I am a bit confused about the scope currently.

  • So there is your new shadow_clone() which scrapes the theme CSS from the upstream repos.

  • then there is your showcase site with all themes and this which shows how to define a function use_theme_github() and then apply it

  • And there is the showcase site which also contains some code

I think all of this together might be a bit too much.

Alternative approach

While reviewing your approach, I was getting a more clear vision on a different approach to tackle this:

  1. Have a single function called use_xaringan_theme() which is called once in the console. It accepts a single theme name (e.g. metropolis as well as a GH repo slug (e.g. pat-s/xaringan-metropolis).
  2. This function does the following:
  • Downloads the CSS from the requested theme and stores it locally under assets/css
  • adds the new CSS file to the YAML header with the correct name
  • Adds a comment into the "setup" chunk stating which revision of the theme is used

I would remove all built-in themes from xaringan and just have a local dictionary stored in the package which tracks the upstream URLs of themes. This would enable using use_xaringan_theme("metropolis") to be the same as use_xaringan_theme("pat-s/xaringan-metropolis") so that users do not have to remember the repo slug

Essentially, if one starts a new presentation, one would do a single call to use_xaringan_theme() and within a sec everything is setup and ready to go.

In addition, there could be a list_themes() functions which list all the themes and optional opens a browser window to your showcase site.

@@ -43,8 +43,7 @@ For some operating systems you may need to
}
\examples{
if (interactive()) {
xaringan::decktape("https://slides.yihui.name/xaringan", "xaringan.pdf",
docker = FALSE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change should not be part of that PR :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops, that's a little accident

#' # remote
#' shadow_clone("tcgriffith/xaringan_theme_example")
#'
shadow_clone = function(theme=NULL, ref="master"){
Copy link
Collaborator

Choose a reason for hiding this comment

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

While the name sounds fancy, I am worried about if it will be descriptive for users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Buuut, it fits the naruto theme!

@tcgriffith
Copy link
Collaborator Author

Hi, @pat-s, your proposed alternative approach looks interesting and sound. After reading your review, I have more thoughts on this.

Downloads the CSS from the requested theme and stores it locally under assets/CSS
adds the new CSS file to the YAML header with the correct name
Adds a comment into the "setup" chunk stating which revision of the theme is used

I'm sold on your approach. Downloading the CSS and modify the yaml seems to be more consistent with the way xaringan handles other assets. My current approach embeds the theme CSS in the html. So if the user wants to make small adjustments to the theme, it will be hard.

have a single function called use_xaringan_theme() which is called once in the console

I think this is a bit inconvenient because it breaks the 1-click "knit" tradition in all types of rmarkdown documents. Instead, we can include the use_xaringan_theme() in the setup chunk and follow the 1-click knit tradition.

I would remove all built-in themes from xaringan and just have a local dictionary stored in the package which tracks the upstream URLs of themes

I disagree. xaringan can ship with some ready-to-use themes which will always work. There are many factors that may affect the individually hosted themes that make them unavailable (changing the GitHub account name/deleting, renaming the repo, etc.) Thus, I tend to keep the existing themes as-is.

then there is your showcase site with all themes and this which shows how to define a function use_theme_github() and then apply it
And there is the showcase site which also contains some code

The showcase website can be adjusted accordingly after the use_theme functionality is settled down
I think it provides a better way to introduce themes to the users but for now, let's just forget about that.

@pat-s
Copy link
Collaborator

pat-s commented Oct 31, 2019

I'm sold on your approach. Downloading the CSS and modify the yaml seems to be more consistent with the way xaringan handles other assets. My current approach embeds the theme CSS in the html. So if the user wants to make small adjustments to the theme, it will be hard.

I'm with you there. Also I do not like having a huge CSS chunk at the bottom of the presentation.
Theme adjustments could to into a separate CSS (e.g. extra.css) also stored in assets/css. Then it is very clear what the theme does and what user modifications have been added.

I think this is a bit inconvenient because it breaks the 1-click "knit" tradition in all types of rmarkdown documents. Instead, we can include the use_xaringan_theme() in the setup chunk and follow the 1-click knit tradition.

I disagree with you on this. I see no difference between calling use_xaringan_theme() once in the console (with all the side effects above) and manually storing the call in the setup chunk.
In contrast, I would not want to have use_xaringan_theme() running on every "knit" of the file. Imo adding a theme should be a one time procedure. This also makes sure that the theme does not get updated in the background but only when the user again calls use_xaringan_theme() (implying that there were upstream changes to the theme meanwhile). Maybe we misunderstood each other on this because I do not see how this approach would break the "1-click knit approach". Maybe you can clarify this one? And in general: What do you think?

I disagree. xaringan can ship with some ready-to-use themes which will always work. There are many factors that may affect the individually hosted themes that make them unavailable (changing the GitHub account name/deleting, renaming the repo, etc.) Thus, I tend to keep the existing themes as-is.

Yes, I see the advantage of built-in themes. The question is just: How do we decide which ones are built-in? Simply all the one up to now?
We could do that but the approach would then not be as 100% clean as to say: "We outsource all themes" and just store a dictionary of upstream theme repos.
But I am also not the one do make such decisions here 😄 .


Even though we have a wall of text now, maybe @yihui can shortly give a summary of his preferences for all of our approaches on this. This way we avoid starting something without the karma of the creator :)

@tcgriffith
Copy link
Collaborator Author

tcgriffith commented Nov 1, 2019

I see no difference between calling use_xaringan_theme() once in the console (with all the side effects above) and manually storing the call in the setup chunk.
In contrast, I would not want to have use_xaringan_theme() running on every "knit" of the file. Imo adding a theme should be a one-time procedure.

I'll explain. My concern is that when the xaringan creator shares the source Rmd (but not the assets). To reproduce the slide, the reader will need to run use_xaringan_theme() first, then knit.

I've seen similar issues on SO that the posted xaringan source failed to compile on my first try. (Apparently I need to run an independent command summon_remark() first.)

OK let's wait for yihui's comment since there are many ways to move forward 😄

@pat-s
Copy link
Collaborator

pat-s commented Nov 1, 2019

I'll explain. My concern is that when the xaringan creator shares the source Rmd (but not the assets). To reproduce the slide, the reader will need to run use_xaringan_theme() first, then knit.

Valid concern! Haven't thought of this one. This is a good example why it is great having more people think about a problem.

Still I think we should find a middle way. Running the whole process of getting the CSS online and storing it on every knit is not efficient and might even cause problems at some point (because internet connectivity would be mandatory all the time if one wants to use themes).

A simple solution could be: Assert that the CSS file from the theme exists in assets/css and then exit the function. And yes, then you are absolutely correct that we need use_xaringan_theme() in the setup chunk which would do exactly this check. Then, if the Rmd is passed around, it would install the CSS on the first "knit".

@tcgriffith
Copy link
Collaborator Author

tcgriffith commented Nov 1, 2019

Still I think we should find a middle way. Running the whole process of getting the CSS online and storing it on every knit is not efficient and might even cause problems at some point (because internet connectivity would be mandatory all the time if one wants to use themes).

Yeah you are right. The logic of use_theme() should be

  • local theme CSS exists: do nothing.
  • No local theme CSS exist: download&setup

@pat-s
Copy link
Collaborator

pat-s commented Mar 14, 2020

@tcgriffith This idea got a bit stale now.
Unfortunately I have no resources to tackle such a big task right now - how do we proceed here? I think it would be important to have Yihui's opinion on this before starting anything in more detail.

@tcgriffith
Copy link
Collaborator Author

@tcgriffith This idea got a bit stale now.
Unfortunately I have no resources to tackle such a big task right now - how do we proceed here? I think it would be important to have Yihui's opinion on this before starting anything in more detail.

I was working on my thesis so I tend to come back for this PR when I have time. On the other hand, yihui said he'd take a look at this thread (personal communication, 2019)but apparently are busy at the moment.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

None yet

3 participants