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

Inject CSS styling into RMD parametrization app #866

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

frankandrobot
Copy link

Modified knit_params_ask so that it is possible to pass custom css/scripts, both as strings and as links to URIs.

Connected to #3945

@@ -203,7 +259,11 @@ knit_params_ask <- function(file = NULL,
params = NULL,
shiny_args = NULL,
save_caption = "Save",
encoding = getOption("encoding")) {
encoding = getOption("encoding"),
html_head_style = c(),
Copy link
Contributor

Choose a reason for hiding this comment

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

One option that won't pollute the argument list so much is to have a single argument that accepts a named list.

html_head = list()

Then, if you want to specify some parts:

html_head = list(style = "body { font-size: 15px; }")

Not sure which would be preferred.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, when I replied in the email, it put the comments in the wrong place.
I initially had the same idea, but as per @trestletech , the long form is preferred.

Copy link
Member

Choose a reason for hiding this comment

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

You could indeed go either way. I agree with @trestletech that R tends to stylistically prefer long argument lists. Even though this isn't normally good form in more well decomposed systems in R where many operations tend to be top level "one button" ones it makes some sense (also forces documentation of the parameters in .Rd as a side benefit).

R/params.R Outdated
@@ -186,6 +186,56 @@ params_namedList <- function() {
empty
}


setup_html_head <- function(html_head_style = c(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: all of our other param helper functions have "params" in their name. It's not the best namespacing, but it's something.

R/params.R Outdated
@@ -194,6 +244,12 @@ params_namedList <- function() {
#' @param shiny_args Additional arguments to \code{\link[shiny:runApp]{runApp}}.
#' @param save_caption Caption to use use for button that saves/confirms parameters.
#' @param encoding The encoding of the input file; see \code{\link{file}}.
#' @param html_head_style a string or a list/vector of strings representing CSS style that will be injected in the HTML HEAD.
#' @param html_head_script a string or a list/vector of strings represeting JS scripts that will be injected in the HTML HEAD.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spelling of representing

html_head_style <- as.vector(html_head_style)
custom_styles <- lapply(html_head_style, shiny::tags$style)

html_head_script <- as.vector(html_head_script)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the as.vector conversion? Am I missing some subtlety?

a <- c(1,2)
lapply(a, function(b) b*2)
lapply(as.vector(a), function(b) b*2)

Copy link
Author

Choose a reason for hiding this comment

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

Again, sorry, when I replied in the email, it put the comments in the wrong place.

the params can be strings or vector of strings.

"body { padding-bottom: 70px; }"
)
## Escape is "cancel" and Enter is "save".
default_script <- shiny::tags$script(shiny::HTML("$(document).keyup(function(e) {\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Any situation where we will want to not inject the defaults?

@frankandrobot
Copy link
Author

As per Jeff, the long form is preferred
On Nov 11, 2016 11:43 AM, "Aron Atkins" [email protected] wrote:

@aronatkins commented on this pull request.

In R/params.R
#866 (review):

@@ -203,7 +259,11 @@ knit_params_ask <- function(file = NULL,
params = NULL,
shiny_args = NULL,
save_caption = "Save",

  •                        encoding = getOption("encoding")) {
    
  •                        encoding = getOption("encoding"),
    
  •                        html_head_style = c(),
    

One option that won't pollute the argument list so much is to have a
single argument that accepts a named list.

html_head = list()

Then, if you want to specify some parts:

html_head = list(style = "body { font-size: 15px; }")

Not sure which would be preferred.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#866 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAwDMQQswdEcuq1_MZv0LmuyKws7_hEmks5q9KlUgaJpZM4Kv-J1
.

@frankandrobot
Copy link
Author

It can be a string or a list of strings
On Nov 11, 2016 11:50 AM, "Aron Atkins" [email protected] wrote:

@aronatkins commented on this pull request.

In R/params.R
#866 (review):

  • ".navbar button { margin-left: 10px; }",
  • Style for the navbar footer.

  • http://getbootstrap.com/components/#navbar-fixed-bottom

  • "body { padding-bottom: 70px; }"
  • )
  • Escape is "cancel" and Enter is "save".

  • default_script <- shiny::tags$script(shiny::HTML("$(document).keyup(function(e) {\n",
  •                                       "if (e.which == 13) { $('#save').click(); } // enter\n",
    
  •                                       "if (e.which == 27) { $('#cancel').click(); } // esc\n",
    
  •                                       "});"
    
  • ))
  • html_head_style <- as.vector(html_head_style)
  • custom_styles <- lapply(html_head_style, shiny::tags$style)
  • html_head_script <- as.vector(html_head_script)

Why the as.vector conversion? Am I missing some subtlety?

a <- c(1,2)
lapply(a, function(b) b_2)
lapply(as.vector(a), function(b) b_2)


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#866 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAwDMWXOWEF7TfeketUJeKSsu0MyYJgUks5q9KrMgaJpZM4Kv-J1
.

@frankandrobot
Copy link
Author

Ok tweaked as requested and added tests. This is re-ready for review @trestletech @aronatkins

Copy link
Contributor

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

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

Could we work through the Connect integration before calling this final? In particular, I'm interested in how we are going to hide the ok/cancel buttons, suppress the enter/escape handling, and inject ok/cancel actions from the parent iframe.

@jjallaire
Copy link
Member

That's fine. We are going to ship a new version of rmarkdown CRAN around
the end of this week (to support the forthcoming IDE patch release). We are
happy to do another rmarkdown release in a month's time if this work misses
the current release.

On Mon, Nov 14, 2016 at 2:24 PM, Aron Atkins [email protected]
wrote:

@aronatkins commented on this pull request.

Could we work through the Connect integration before calling this final?
In particular, I'm interested in how we are going to hide the ok/cancel
buttons, suppress the enter/escape handling, and inject ok/cancel actions
from the parent iframe.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#866 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGXx_bSea28lbgY8PhA7v1ctaNuuAZbks5q-LVWgaJpZM4Kv-J1
.

@frankandrobot
Copy link
Author

frankandrobot commented Nov 14, 2016

Could we work through the Connect integrati I'm interested in how we aregoing to hide the ok/cancel buttons, suppress the enter/escape handling,and inject ok/cancel actions from the parent iframe.

This code is designed to override the defaults with the custom. So for
example, to supress the handling, if I recall correctly, it's just an event
that can be easily over ridden. The hiding is similar via css.

What is the use case to insert actions? In any case, all you do is insert
JavaScript to handle the actions.

Update: after taking a better look at the code, I noticed that there might be enough logic in the shiny R code for inputs, to keep this strategy from working for certain use cases, for example, default (blank) param support. @aronatkins we should touch base in the am so i can get a better idea of what the use cases are

@@ -250,6 +250,7 @@ params_html_head <- function(html_head_style = c(),
#' You must take care to unsure that the URL is absolute.
#' @param html_head_script_link same as above except that these are interpreted as SRC attributes in SCRIPT tags.
#' You must take care to unsure that the URL is absolute.
#' @param disable_bootstrap if true, does not inject boostrap scripts and styles in the HTML HEAD.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the common formatting is for this package, but you could consider styling with if \code{TRUE}, does not... Probably more important to align with the conventions of the package but just making sure you're aware of the option.

@yihui yihui force-pushed the master branch 2 times, most recently from 9ce8b28 to 2e8b846 Compare May 30, 2020 03:35
@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.


Uriel Avalos seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
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.

5 participants