emillykkejensen commented 7 years ago

Hi

I was wondering, what I need to do for ECharts2Shiny to work with HTML ui?

XD-DENG commented 7 years ago

Hi, thanks for approaching.

May you share more about what you mean by "HTML ui"? Thanks.

emillykkejensen commented 7 years ago

Well I allready found an solution :-)

With Shiny UI:

App.R:

library(shiny)
library(ECharts2Shiny)
server <- function(input, output) {
  renderWordcloud(div_id = "test", data = c('A', 'A', 'A', 'B', 'B', 'C', 'A'))
}
ui <- fluidPage(
  loadEChartsLibrary(),
  div(id="test", style="width:100%;height:500px;"),
  deliverChart(div_id = "test"),
  title = "Shiny UI version"
)
shinyApp(ui = ui, server = server)

With HTML UI:

Server.R:

library(shiny)
library(ECharts2Shiny)
server <- function(input, output) {
  renderWordcloud(div_id = "test", data = c('A', 'A', 'A', 'B', 'B', 'C', 'A'))
}

www/index.html

<!DOCTYPE html>
<html>
<head>

<meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
<meta name="viewport" content="width=device-width, initial-scale=1" />

<title>HTML version</title>

<link href="shared/shiny.css" rel="stylesheet" />
<link href="shared/bootstrap/css/bootstrap.min.css" rel="stylesheet" />

<script type="application/shiny-singletons"></script> 
<script src="shared/json2-min.js"></script>
<script src="shared/jquery.min.js"></script>
<script src="shared/babel-polyfill.min.js"></script>
<script src="shared/shiny.min.js"></script>

<script src="shared/bootstrap/js/bootstrap.min.js"></script>
<script src="shared/bootstrap/shim/html5shiv.min.js"></script>
<script src="shared/bootstrap/shim/respond.min.js"></script>

<script src="echarts.js"></script>

</head>

<body>
  <div class="container-fluid">
    <div id="test" style="width:100%;height:500px;"></div>
    <div id="test" class="shiny-html-output"></div>
  </div>
</body>

</html>

and then add echarts.js to the www/ library

This works, and shows ECharts2Shiny with HTML ui.

However I would like it to not wrap it in 'fluidPage' in the renderWordcloud function - line 124 in word_cloud.R. Generally I think, it would be better to just return the 'js_statement' and then aks the users to enclose it into a renderUI function - like:

output$wordcloud <- renderUI({
  renderWordcloud(data = c('A', 'A', 'A', 'B', 'B', 'C', 'A'))
})

XD-DENG commented 7 years ago

Hi @emillykkejensen , thanks so much for exploring and share!

I had the current design as I thought it may be good to include output$wordcloud <- renderUI({ in the function itself so that the users don't have to type it explicitly repeatedly. Given this package has been published for some time and is being used by some people (actually not many LOL), so it may be hard to change the API and the relevant design.

But do let me know if you have any other good suggestions! I would be more than happy to refine it with your input.

emillykkejensen commented 7 years ago

Okay - here's an idea:

1) Add: 'wrap_in_renderUI = TRUE' to the functions arguments 2) Replace:

to_eval <- paste("output$", div_id ," <- renderUI({fluidPage(tags$script(\"",
                     js_statement,
                     "\"))})",
                     sep="")

(line 124-127) with:

if(wrap_in_renderUI){
    to_eval <- paste("output$", div_id ," <- renderUI({fluidPage(tags$script(\"",
                     js_statement,
                     "\"))})",
                     sep="")
  } else {
    to_eval <- paste("tags$script(\"", js_statement, "\")")
  }

just tried it locally and it work perfect :)

XD-DENG commented 7 years ago

Then how should we set the default value for wrap_in_renderUI? If the default value if "FALSE", it will break the current APE; If it's "TRUE", then users always need to specify wrap_in_renderUI = FALSE if they want to type "output$..." by themselves, which may be a burden as well.

If there is no predominant advantage, I may prefer to keep current design.

emillykkejensen commented 7 years ago

It not a buden to write 'wrap_in_renderUI = FALSE' - it's a burden if one can't use ECharts2Shiny with HTML ui. By defaulting wrap_in_renderUI to TRUE, no change is made to the users currently using the function - but by including this extra argument, more people are able to use it.

There really is no downside to it and there is a major advantage with it - the posibility to use ECharts2Shiny with HTML ui or with other Shiny ui's not based on the fluid layout (eg. fixed layout, flex layout, split layout etc. that do not use fluidPage()) :-)

XD-DENG commented 7 years ago

I understand the point you're trying to make now: enabling ECharts2Shiny with HTML ui (www/index.html) etc.

Kindly allow me some time to check the code again and see how we can move forward with the inspiration of your idea. Will get back to you on this for sure ;-)

emillykkejensen commented 7 years ago

Sounds good :-)

XD-DENG commented 7 years ago

Hi @emillykkejensen , I have checked the codes and please check the findings as below ;-)

I must admit I've made a "mistake" in statement

to_eval <- paste("output$", div_id ," <- renderUI({fluidPage(tags$script(\"",
                   js_statement,
                   "\"))})",
                   sep="")

Actually we don't need to include fluidPage at all. The statement without it like below will definitely work.

to_eval <- paste("output$", div_id ," <- renderUI({tags$script(\"",
                   js_statement,
                   "\")})",
                   sep="")

I've made this change in branch dev_2, commit f7ecc1f7c7f5381fc4e1cd26240abc0fc50ee458, to all the relevant .R files in /R folder.

Then let's go back to your question: how we can make this package work better with HTML ui. Actually with the current codes, you can already use different layouts (fixed layout, flex layout, split layout etc, as you pointed out). The command above will render the in the final applicaiton, what we need to do is only to prepare the corresponding <div>. We had the confusion only because I made the stupid mistake to have included fluidPage in the command above while we don't need it actually.

Hence your question is also solved: we are able to adopt diverse layouts while we don't need to make big change. We don't need to bother to add the argument wrap_in_renderUI either.

Please let me know your thoughts.

Thanks again for reaching out so that I can find the mistake I made.

emillykkejensen commented 7 years ago

That could work. However I would like to stick to shiny's intended way of doing things, where you write the output to the server.R like: output$wordcloud <- renderUI(renderWordcloud(data = c('A', 'A', 'A', 'B', 'B', 'C', 'A')))

The main reason, is that I'm doing some work on my data before I put it into the renderWordcloud function, and I would like to do that work inside a reactive expression.

XD-DENG commented 7 years ago

Hi @emillykkejensen , for this request, I still think the current design is "good enough". My mainconcern is that it may increase the complexity if we try to have a reactive expression and do data manipulation inside it. As you may have found, the current design is quite straightforward :"translate" R data object into Javascript statements. I think it's better not to touch it.

I hope you can kindly understand this.

I'm not sure about your specific need but possibly you may want to refer to this example example-5 Use Reactive Values.

XD-DENG / ECharts2Shiny

HTML ui #45

With Shiny UI:

With HTML UI: