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

f/ioslides background #687

Merged
merged 1 commit into from
Sep 6, 2017
Merged

Conversation

kiwiroy
Copy link
Contributor

@kiwiroy kiwiroy commented May 2, 2016

Change

Add more simple syntax for slide backgrounds and slide specific class.

Issues #195 and #230 are addressed, or partially so. The thank you slide is a little tricky as it breaks the mold somewhat.

Usage

## Slide One {data-background="http://www.prolificbeing.com/wp-content/uploads/2015/02/Hello-World.png"}
## Another Slide | subtitle
Lorem ipsum dolor.
## Final content {slide-class="csstricks"}
`
slide > slide [data-slide-num="7"] {
  background-image: url("figures/xx.jpg");
}
`
## Thanks for listening {slide-class="thank-you-slide" class="flexbox vleft auto-fadein"}

Testing

@alequartz's unit testing would be a really useful addition.

@kiwiroy
Copy link
Contributor Author

kiwiroy commented May 9, 2016

Is there anything I can do to increase the merge ability of this?

@jjallaire
Copy link
Member

The data-background stuff looks great. On the slide-class, I'd rather see the syntax for this be something like:

Heading {.css-tricks}

To make this work I think you'd need to let pandoc apply the class to the section div and then use jquery at startup to move classes (that aren't section, level2, etc.) from divs onto their enclosed slide element.

@alequartz
Copy link

I have updated my pull request. Maybe you could append you tests to the test-ioslides.R

@kiwiroy
Copy link
Contributor Author

kiwiroy commented May 19, 2016

@jjallaire thanks for the input. The Heading {.css-tricks} is already applied by pandoc, but the id and class attributes are applied to the <article> tag, by the lua code. I was not sure this duality could be resolved simply, but I like the simplicity of what you have suggested. In an attempt to capture that simplicity and build on it, I've augmented the PR. The new usage is thus:

## Heading {.css-tricks..}

The trailing `..` attempts to suggest parent tag as in parent directory :smile:

The trailing `..` are removed so the `.css-tricks` selector must be used in a stylesheet.

`
.css-tricks > hgroup {
  background-color: rgb(100, 100, 100);
  margin-top: -40px;
  padding-top: 40px;
  margin-left: -60px;
  padding-left: 60px;
  margin-right: -60px;
  padding-right: 60px;
}
`

If this syntax is acceptable and likely to be merged I can prepare a pull request against the gh-pages branch.

@alequartz excellent, I've refactored slightly to allow for easier additional testing.

@jjallaire
Copy link
Member

To be honest I don't love the .. syntax (it seems obscure to me). Standard
pandoc behavior is to attach classes and data attributes to the section tag
not to the header tag so I think this is the behavior that users will
expect. If it's too tricky to do this in Lua code then you can pretty
easily do it with jquery on page load.

On Wed, May 18, 2016 at 10:52 PM, Roy Storey notifications@github.com
wrote:

@jjallaire https://github.com/jjallaire thanks for the input. The Heading
{.css-tricks} is already applied by pandoc, but the id and class
attributes are applied to the

tag, by the lua code
https://github.com/rstudio/rmarkdown/blob/master/inst/rmd/ioslides/ioslides_presentation.lua#L264-L266.
I was not sure this duality could be resolved simply, but I like the
simplicity of what you have suggested. In an attempt to capture that
simplicity and build on it, I've augmented the PR. The new usage is thus:

Heading {.css-tricks..}

The trailing .. attempts to suggest parent tag as in parent directory 😄

The trailing .. are removed so the .css-tricks selector must be used in a stylesheet.

.css-tricks > hgroup { background-color: rgb(100, 100, 100); margin-top: -40px; padding-top: 40px; margin-left: -60px; padding-left: 60px; margin-right: -60px; padding-right: 60px; }

If this syntax is acceptable and likely to be merged I can prepare a pull
request against the gh-pages branch.

@alequartz https://github.com/alequartz excellent, I've refactored
slightly to allow for easier additional testing.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#687 (comment)

@kiwiroy
Copy link
Contributor Author

kiwiroy commented May 19, 2016

@jjallaire it isn't that it is difficult to achieve in Lua, rather the syntax is a challenge and why my original choice was to add an attribute using the same technique for the {data-background=#CCC}. Whether jQuery or Lua achieve the assignment / movement of the class, the syntax for achieving this needs to be obvious for the user and easily differentiated for the code - yes I'd like that flexibility. As an example

## Heading One {.flexbox}
Lua sees `attr["class"] = "flexbox"`

## Heading Two {slide-class="thank-you-slide" class="flexbox vleft auto-fadein"}
Easily separated, Lua gets `attr["slide-class"] and attr["class"]`

## Heading Three {class="thank-you-slide.. flexbox vleft auto-fadein"}
Like the previous slide, easily separated, concise, but granted, obscure

347e518 removes this to focus on the data-background code as the branch name suggests.

@kiwiroy
Copy link
Contributor Author

kiwiroy commented May 23, 2016

Just realised the code was not honouring self_contained: true hence 26f0e07.

@kiwiroy kiwiroy force-pushed the f/ioslides-background branch from 26f0e07 to 6d10665 Compare May 31, 2017 01:23
@yihui yihui added this to the v1.7 milestone Aug 30, 2017
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Sorry for leaving this PR here for over a year. I spent a couple of hours reviewing and testing it. It looks pretty good to me, and I think it is very useful to many other people, too. I only expect a quick revision, but I'm not sure if you are still interested. I can also make the revision by myself. Thanks!

@@ -117,10 +117,14 @@ process_html_res <- function(html, reg, processor) {
}

process_images <- function(html, processor) {
Copy link
Member

Choose a reason for hiding this comment

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

process_images() is called by html_document_base(), so the change in this function has potential risk of affecting all HTML output formats. I'd rather be a little conservative, and move the new process_html_res() call you added to the end of base64_encode_images() (line #140), so that this only affects ioslides_presentation(), because only ioslides_presentation() calls base64_encode_images().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yihui sorry I didn't see this update. No worries on the timing, I've been reporting using a branch that includes it in the intervening time. Good to see it merged. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

No worries. Thanks!

@jaredlander
Copy link

Been playing with this and it is a great solution. One additional request though. this sets background-size: 100% 100%; inline, for the slide so that global css can't override.

Can we have an option to set other background-size and background-repeat etc?

For example, my background images look better with background-position: center; background-repeat: no-repeat; background-size: contain;.

@yihui
Copy link
Member

yihui commented Oct 26, 2017

@jaredlander Okay, 70a1847 was my wedding gift to you. Now background-size defaults to contain, and background-position defaults to center. If you want to change them, just use the attributes data-background-size and data-background-position.

@jaredlander
Copy link

jaredlander commented Oct 27, 2017

Thank you @yihui this makes both me and @beckmart very happy. An awesome gift!

@jaredlander
Copy link

Not sure if this is related or if it was brought up elsewhere, but with the latest version of rmarkdown (installed from GitHub to get the background slide functionality), knitr::include_graphics no longer gets URI encoded. I doubt this is related to background images, but it's an interesting change? Was it by design or unintended?

@yihui
Copy link
Member

yihui commented Nov 12, 2017

@jaredlander Could you provide a minimal reproducible example as a zip archive? You probably want to use knitr::image_uri() instead.

@yihui
Copy link
Member

yihui commented Nov 13, 2017

@jaredlander Could you test the latest devel version here? I think your issue might be the same as #1197, which I just fixed. Thanks!

@jaredlander
Copy link

@yihui I'm working on a talk now. Will update when I get in the air and report back.

@jaredlander
Copy link

Looks good now! @yihui

@yihui
Copy link
Member

yihui commented Nov 13, 2017

@jaredlander Glad to hear that!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants