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

Make autocomplete compatible with shiny modules #54

Merged
merged 1 commit into from
Apr 6, 2018
Merged

Make autocomplete compatible with shiny modules #54

merged 1 commit into from
Apr 6, 2018

Conversation

GregorDeCillia
Copy link
Contributor

Attempt to resolve issue #49

@vnijs
Copy link
Collaborator

vnijs commented Apr 6, 2018

Thanks @GregorDeCillia. Although I think that autocompletion inshinyAce should support modules, I am concerned about possibly breaking others' applications if we change the default structure of the input string used. I'd much rather do something like add an additional argument to aceAutocomplete. I'm cc-ing @saurfang since he is the expert on the autocomplete functionality in shinyAce in case he has additional ideas.

Honestly, I still don't see why modules don't work here if all they do is change "editor" to "sessiona_editor" as that should just be the new inputId. Perhaps I'm missing something.

@saurfang
Copy link
Contributor

saurfang commented Apr 6, 2018

I was able to verify this fix. It looks Shiny makes assumption that all inputs belonging to a module share the module id prefix. If we list input names inside module server function, we only get list of inputs that have the module prefix with their prefix already stripped out.

While I think there is way to make this fix backwards compatible but it would be too complicated for it to be worthwhile. (i.e. we pass along namespace prefix into the widget and reconstruct the current hint input naming convention.)

I'm supportive of this breaking change since I suspect very few people use shinyAce_inputid_hint explicitly. Can you add this breaking change in NEWS.md (and maybe bump the version?)

@vnijs vnijs merged commit e88cd9e into trestletech:master Apr 6, 2018
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.

3 participants