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

Functions to Refactor #163

Open
1 task
ramnathv opened this issue Jul 17, 2013 · 8 comments
Open
1 task

Functions to Refactor #163

ramnathv opened this issue Jul 17, 2013 · 8 comments

Comments

@ramnathv
Copy link
Owner

  • render_template

I want to overload this function so that it takes a string or a filename for the template

render_template = function(template, data = parent.frame(1),...){
  if (file.exists(template)) {
    template <- read_file(template)
  }
  paste(capture.output(
    cat(whisker.render(template, data = data))
  ), collapse = "\n")
}

I can enhance this function by checking for the template in multiple locations. The default is the current path, and if not found it could search in the root path of the package, or libraries/:lib. This would allow more compact code.

@ramnathv
Copy link
Owner Author

  • setLib

I should enhance this method, so that all library related settings that were initialized get re-initialized. Here is how the new setLib method would look.

setLib = function(lib, ...){
  lib <<- lib; LIB <<- get_lib(lib)
  templates <<- modifyList(list(
    page = 'rChart.html', 
    chartDiv = NULL, 
    script =  file.path(LIB$url, 'layouts', 'chart.html')
  ), list(...))
}

@ramnathv
Copy link
Owner Author

  • print

The print method includes chartDiv in order to use with Rmd docs. However, the code I have currently is highly convoluted as can be seen below.

if (is.null(templates$chartDiv)){
  chartDiv =  sprintf("<%s id='%s' class='rChart %s'></%s>", 
      container, params$dom, LIB$name, container)
} else {
  chartDiv = render_template(templates$chartDiv, list(chartId = params$dom))
}

I can rewrite it so that in initialize(), I set

templates$chartDiv <<- "
   <{{container}} id = '{{ chartId }}' class = 'rChart {{ lib }}'>
   </{{ container}}>
"

Once render_template is refactored as above, I can just do

chartDiv = render_template(templates$chartDiv, list(
  chartId = params$dom,
  lib = LIB$name,
  container = container
))

@ramnathv
Copy link
Owner Author

  • renderChart

Currently, renderChart only adds the javascript code to create a chart. The div containing the chart is created using ShinyUI. This is restrictive, since some chart types contain complicated divs (e.g. Rickshaw). Now, the print method I use for embedding charts in a slidify document print both the div related HTML and the javascript. I can use the print method instead of html in renderChart and use the div created using ShinyUI merely as a wrapper. This simplifies renderChart considerably and at the same time makes it more flexible. Here is an implementation that I have done some preliminary tests on

renderChart2 <- function(expr, env = parent.frame(), quoted = FALSE) {
  func <- shiny::exprToFunction(expr, env, quoted)
  function() {
    rChart_ <- func()
    cht_style <- sprintf("<style>.rChart {width: %spx; height: %spx} </style>",
      rChart_$params$width, rChart_$params$height)
    cht <- paste(capture.output(rChart_$print()), collapse = '\n')
    HTML(paste(c(cht_style, cht), collapse = '\n'))
  }
}

@ramnathv
Copy link
Owner Author

show

I can overload the show method so that charts can be saved as html files and embedded as iframe. This would solve a lot of issues related to interference between js files. Here is a simple implementation using options. I tested it and it works. I need to refactor the show method so that all the overloaded functions are consistent and are controlled in more or less the same manner.

if (!is.null(getOption('rcharts.vis.tag')) && 
  getOption("rcharts.vis.tag") == 'iframe'){
    file_ = sprintf("assets/img/%s.html", params$dom)
    .self$save(file_)
    cat(sprintf("<iframe src=%s></iframe>", file_))
    return(invisible())
}

Here is a list of ways in which the show method can work currently.

  1. static = T: save to a temporary file and open as static url
  2. static = F: open chart as a shiny app
  3. getOption('knitr.in.progress') == T: add lib assets and print chartDiv + script inline.
  4. getOption('rcharts.vis.tag') == 'iframe': save as html file and embed as iframe

@reinholdsson, @timelyportfolio, if you have any thoughts on how to make these consistent and easy to use, post your ideas here. This would be an important refactoring, and I want to get it right.

@thorei
Copy link

thorei commented Jul 22, 2013

thorei is someone different - please address the right person.

@ramnathv
Copy link
Owner Author

@thorei My apologies. I confused his twitter handle with his github handle.

@ramnathv
Copy link
Owner Author

getMode

This function chooses the best mode for the chart to be displayed

NOTES:

  • change rcharts.mode to rcharts.show.mode to allow consistency
  • add utilitiy function
`%?=%` <- function(a, b){
  ifelse(!is.null(a), a == b, FALSE)
}
getMode = function(mode_){
  # if mode_ is specified, just return it
  if(!is.null(mode_)){
    return(mode_)
  }
  # if rcharts.mode is specified, just return it
  if(!is.null(getOption('rcharts.mode'))){
    return(getOption('rcharts.mode'))
  }
  # if knitr is in progress, return mode = iframe, else static
  if(getOption('knitr.in.progress') %?=% TRUE){
    mode_ = 'iframe'
  } else {
    mode_ = 'static'
  }
  return(mode_)
}

@ramnathv
Copy link
Owner Author

A more compact version of getMode. It works by picking a default based on whether or not knitr is in progress. It then cycles through mode_, rcharts.mode and the default, returning the first non-null value that it encounters.

getMode = function(mode_){
  default = ifelse(getOption('knitr.in.progress') %?=% TRUE, 'iframe', 'static')
  mode_ = mode_ %||% getOption('rcharts.mode') %||% default
  return(mode_)
}

Would it be better to use rcharts.mode as a global option or make it a chunk option, that can be set inside the knitr document? One advantage of using opts_chunk is that its effects, while global inside the document, are local to it, thereby preventing unwanted side effects in the R console.

I can then think of having a set of options to control rcharts.

  • rcharts.show: 'inline', 'iframe'
  • rcharts.mode: selfcontained, standalone, draft

This sounds very promising.

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

No branches or pull requests

2 participants