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

[Completion] Fix AutoCompleteList in Constructor #52

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

saurfang
Copy link
Contributor

@saurfang saurfang commented Apr 3, 2018

HTML data-* attributes in htmltools no longer automatically converts to JSON from list and attribute key is converted to lower cases. This commit changes the attribute name to lower case and perform explicit JSON conversion. The example has also been fixed for the error that anonymous function needs wrapping braces.

Fixes #48

HTML data-* attributes in htmltools no longer automatically converts to JSON from list and attribute key is converted to lower cases. This commit changes the attribute name to lower case and perform explicit JSON conversion.

Fixes trestletech#48
@vnijs
Copy link
Collaborator

vnijs commented Apr 3, 2018

Thanks @saurfang! I will review and merge tomorrow. Any chance you could add one (or more) simple tests to help safeguard against similar changes (e.g., in htmltools or even jsonlite) in the future?

@saurfang
Copy link
Contributor Author

saurfang commented Apr 4, 2018

Probably will be good to add some tests with shinytest. I might have some time to work on it over the weekend. Feel free to merge this first to unblock if you wish.

@vnijs vnijs merged commit 89037d6 into trestletech:master Apr 4, 2018
@vnijs
Copy link
Collaborator

vnijs commented Apr 4, 2018

That sounds great @saurfang! I have been trying out this feature (very nice!) and I have a few questions:

  1. Is it possible to turn off suggestions from local? Or else to remove certain things from it? For example, for a yaml file, I only want to suggest previous labels (which may have spaces) but not the values (e.g., 1000) or the individual label pieces. See Model > Decision analysis @ https://vnijs.shinyapps.io/radiant/ for an example. There I'd want to show "Sign with Movie Company" but not "Sign" separately.
  2. I have to look into this a bit more but when testing in a larger app it seems that an autoCompleteList set in the call to aceEditor is overwritten when using updateAceEditor with a different autoCompleteList. Is that correct? If so, would be nice to have an option to combine the two lists.

@vnijs
Copy link
Collaborator

vnijs commented Apr 4, 2018

@saurfang Here is an example of (2) in my previous comment. fun1 and fun2 are shown when autoCompleteList is provided in the call to aceEditor. fun1 and fun2 are not available when the observe is used however.

For (1) it seems it should be possible to ignore "local" suggestions in Ace (see link below) but I wasn't clear on the implementation in R / Shiny. Any ideas?

https://stackoverflow.com/questions/41085240/is-there-a-way-to-remove-all-the-default-autocomplete-suggestions

library(shiny)
library(shinyAce)

shinyApp(
  ui = basicPage(
    aceEditor(
      "auto_editor", 
      "plot(wt ~ mpg, data=mtcars)\n", 
      mode = "r",
      autoComplete = "live",
      autoCompleteList = list(test = c("fun1", "fun2"))
    )
  ), 
  server = function(session, input, output) { 
    ## without the observe below 'fun1' and 'fun2' are shown
    ## however with the observe they are not available
    observe({
      comps <- list(mtcars = colnames(mtcars), airquality = colnames(airquality))
      shinyAce::updateAceEditor(session, "auto_editor", autoCompleteList = comps)
    })
  }
)

@saurfang
Copy link
Contributor Author

saurfang commented Apr 5, 2018

@vnijs I think both are good improvements. Coincidentally I happened to be working on an SQL editor as an RStudio add-in / Shiny App and the autocompletion in shinyAce needs to be a lot more configurable. For your (1), it thinks it makes the most sense to turn off local completer completely, so you can implement separate completer from R side by parsing YAML file to extract keys.

@vnijs
Copy link
Collaborator

vnijs commented Apr 5, 2018

Did you post something about the SQL editor already? I seem to recall seeing something about it but didn't see it on your GitHub page. Sounds really interesting!

Parsing the yaml with R is easy enough and passing it to the completer works fine as well. Is it already possible to turn off the local completer with the currently available code or would it require additional JS?

@saurfang
Copy link
Contributor Author

saurfang commented Apr 5, 2018

That thing was still very much a work in progress so I made it to private for the time being. I just put out a PR to unblock you but it might need a bit more work to be releasable quality.

@saurfang
Copy link
Contributor Author

saurfang commented Apr 5, 2018

oh and re: (2), this is where we can potentially merge instead of replace https://github.com/trestletech/shinyAce/blob/master/inst/www/shinyAce.js#L137 do you have a clever function interface in mind? We can add one more argument to the function or maybe create a new function that handles static completion configuration only. I feel updateAceEditor function is doing too much and might be confusing.

@vnijs
Copy link
Collaborator

vnijs commented Apr 5, 2018

Sounds really nice! Will look in detail tomorrow. FYI based on a quick tryout I noticed the message below when using examples/06-autocomplete ...

image

@saurfang
Copy link
Contributor Author

saurfang commented Apr 5, 2018

oops. that particular one should be fixed now.

@vnijs
Copy link
Collaborator

vnijs commented Apr 5, 2018

The example seems to work now!

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.

autoCompleteList in aceEditor can not work
2 participants