Add shiny inputs

[crew102]

Hey @cpsievert , this PR addresses the issues discussed in #45. Specifically, it:

1. Changes the style of any clicked term to be underlined... Clicked topics are given a thicker border.
2. Updates the shiny input object to know about clicks to the terms/topics. I decided not to not link hover events to the shiny input object.

A few notes regarding the behavior of input$ldavis_term_clicked and input$ldavis_topic_clicked:

• ldavis_term_clicked is reset to null whenever ldavis_topic_clicked changes
• clearing the vis results in ldavis_term_clicked and ldavis_topic_clicked going to null
• the only time ldavis_topic_clicked can be null and ldavis_term_clicked non-null is when viewing the marginal distribution (i.e., when no topic is selected)
• changing topics via the topic input widget doesn't change ldavis_topic_clicked. In other words, you have to actually click on a topic (or reset the vis) to change ldavis_topic_clicked.

The first three items describe desired behavior from my perspective (in other words, this was what I was going for). Ideally the topic input object would change ldavis_topic_clicked, but I didn't have time to implement it so...

[cpsievert]

I don't think you need the observe() blocks here, I'd prefer to see:

output$topicClicked <- renderPrint({
  if (is.null(input$ldavis_topic_clicked)) return()
  paste("You clicked on topic:", input$ldavis_topic_clicked)
})

[cpsievert]

Have you tested this functionality outside of a shiny context? I think this would throw an error since Shiny is not defined if it's not running in a shiny app. In that case, we should add a check to see if we're in shiny mode similar to how HTMLwidgets does it -- https://github.com/ramnathv/htmlwidgets/blob/master/inst/www/htmlwidgets.js#L15

[crew102]

I haven't..I'll add a check like the one you describe before calling onInputChange().

[cpsievert]

Thanks @crew102, this is looking great! As long as you address my small comments, this is very close to getting a 👍 from me, but I also wait to see if @kshirley has any feedback before merging...

Ideally the topic input object would change ldavis_topic_clicked, but I didn't have time to implement it so...

I very much agree, and have a feeling @kshirley would as well. Any idea how hard it would be to implement? If it isn't super difficult, I may insist on it since our general philosophy has been that clicking on a topic yields the same behavior as flipping through them via the input buttons.

[crew102]

Any idea how hard it would be to implement?

It's actually easy. I'll resubmit with it implemented.

Also there was a small bug in this version of the PR where clicking on a topic, clearing the vis, then clicking on that same topic again was not resulting in the topic's border being bolded. I've fixed this.

[crew102]

OK, I think this should be all set now. I was having some git difficulties but I think this should do it.

[cpsievert]

Sorry, I just thought of this, but it seems like we should only modify the stroke/underline when in shiny mode. Otherwise, it might be confusing as to why terms are getting underlined on click (when, outside of shiny mode, it doesn't really do anything)

So, instead of checking for inShinyMode just before calling Shiny.onInputChange(), I think I'd prefer it if term_click() and topic_click() exited early

topic_click = function(newtopic, newtopic_num) {
  if (inShinyMode) {
    return null;
  }

  ...
}

[crew102]

Sure, that makes sense.

[cpsievert]

Ideally the input name should have information about the relevant outputId (so, on the server, you can listen/respond to events from a specific output). Ideally the API would work like leaflet, but I'm not immediately sure how to implement, any ideas?

[crew102]

I think this should be easy, as the outputId gets passed into the LDAvis code as a selector here:

LDAvis/inst/shiny/shinyLDAvis.js

Lines 13 to 14 in bdd7b01

	var to_select = "#" + el.id;
	var vis = new LDAvis(to_select, json_file);

Note that we would need #39 to be merged to allow handling multiple ldavis outputs in the same app.

I'll change the API as you suggested, and whenever #39 is merged than having the outputIds in the input names will come in handy.

[cpsievert]

Nice, thanks! I'd prefer more descriptive variable names here, say shinyClickedTopic/shinyClickedTerm

Once that's done, this looks good to me!

[crew102]

done!

[kshirley]

Just to chime in: this feature sounds good and from the discussion it looks like you two are covering all the bases.

I give it my thumbs-up when complete. Looking forward to trying it out.

Thanks for contributing @crew102 !

And thanks as always for the leadership and responsiveness, @cpsievert .

[cpsievert]

@crew102 one last thing if you'd like -- update the NEWS file and add yourself as a contributor in the DESCRIPTION 😃

[crew102]

Hey @cpsievert , I tried to follow the style that I found in NEWS, but I could have gotten it wrong. Also, I incremented the version to 0.3.5.