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

Allow usage of shinyTree in modules, closes #81 #86

Closed
wants to merge 1 commit into from
Closed

Allow usage of shinyTree in modules, closes #81 #86

wants to merge 1 commit into from

Conversation

thothal
Copy link
Contributor

@thothal thothal commented Aug 27, 2019

This closes #81.

Dashes were replaced by underscores in the HTML generating function and in the respective JS file, which refers to this. An example was added based on the reprex in #81.

Ids in shiny modules are namespaced by prepending the modules id and a
dash "-". The shinyTree id is used "as is" as a JavaScript variable
name in the head of the HTML file, which causes a bug, because the dash
is an invalid character for variable names.

The dash was consequently replaced by an underscore "_" in the HTML
generating R function as well as in the JavaScript file which
references this variable.
@trafficonese
Copy link
Contributor

I did not spot the difference to #85.

And using your PR I am not seeing the icons rendered correctly. So the problem with #85 persists or am I wrong?

@thothal
Copy link
Contributor Author

thothal commented Aug 27, 2019

If I run

library(shiny)
runApp(system.file("examples/18-modules", package="shinyTree"))

I get this:
shinytree

So the icons should be visible. And in #85 you missed to fix the shinyTree.js. So your code did not show any error message any more, because the <script> was now properly set up, but in the JavaScript it still tried to access it via the old name. That's why in your PR the icons worked not via the types argument.

@trafficonese
Copy link
Contributor

trafficonese commented Aug 27, 2019

All clear, I had not installed your PR at all.

I thought
devtools::install_github('shinyTree/shinyTree', pull=86, ref=NULL)
would do it, but it actually just installed the master branch.
So with
remotes::install_github("shinyTree/shinyTree#86")
I managed to get your PR and indeed the icons show correctly.

Got it, thanks.
So #85 can actully be closed right?

@trafficonese
Copy link
Contributor

trafficonese commented Aug 27, 2019

You got me quite confused now :)

In #85 there are also changes in the JS file although a bit different than yours, but I think they achieve the same thing.

This is from 85

      var elidtypes = el.id.replace("-", '_');
      if (typeof window[elidtypes + "_sttypes"] !== 'undefined'){
      	sttypes = window[elidtypes + "_sttypes"];
      }

And this is from 86

      if(typeof window[el.id.replace("-", "_") + "_sttypes"] !== 'undefined'){
        sttypes = window[el.id.replace("-", "_") + "_sttypes"];
      }

And running your example with my PR does work aswell. So maybe I had a mistake in the example code?

@thothal
Copy link
Contributor Author

thothal commented Aug 27, 2019

Hmm, strange, I swear I did not see that in the beginning. That was IMHO the reason why the icons did not show up. But I am pretty new to GitHub so I could very well be that I overlooked something.

So in this case it is exactly the same thing, and maybe the issue was indeed in the example code. In any case, happy that the issue is solved. I will close my PR in this case then. Sorry for the confusion I may have caused.

@thothal thothal closed this Aug 27, 2019
@trafficonese
Copy link
Contributor

If you dont mind, I would take your example and include it in my PR, so there are 2 different examples of using shinyTree in modules, whereas your example is the way to define types correctly.

I would include you as contributor aswell, then this PR could be closed. You're ok with that?

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.

types do not work in shiny modules
2 participants