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

Add callback function to handle switching tabs in multi-tab apps #30

Merged
merged 2 commits into from Apr 24, 2018

Conversation

@crew102
Copy link
Contributor

@crew102 crew102 commented Apr 4, 2018

  • Should close #17.
  • Example usage can be found at inst/examples/switchTabs.R
  • This solution should work when needing to switch menuitems in shinydashboards as well, though I didn't test this.
@carlganz
Copy link
Owner

@carlganz carlganz commented Apr 8, 2018

@crew102 thanks for taking the time to work on this. I think the file manipulation, and text parsing in readCallback and getJSfunBody are way outside the scope of what this package should do.

When I get some free time I will integrate the switchTabs function you provided. Thank you very much for the contribution.

@crew102
Copy link
Contributor Author

@crew102 crew102 commented Apr 9, 2018

Hi @carlganz , readCallback() just reads in a callback function defined in inst/javascript/callbacks - it doesn't really add much scope to the package...and getJSfunBody() is just a helper function (it's not exported), so I don't see how that adds scope either.

I assume you are thinking of just putting the body of the switchTab() function in an R string and exporting that for users? The reason why I chose to have readCallback() as the public API for the functionality added in switchTab() instead of just exporting a string was a) I generally think that different languages shouldn't be mixed in the same source code file if you can help it (hence the rationale for reading in a javascript file), and b) I think it will be easier for interested users to understand what switchTab() actually does if it appears in its full form.

@carlganz carlganz merged commit 8117dee into carlganz:master Apr 24, 2018
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 0% of diff hit (target 31.7%)
Details
codecov/project 22.8% (-8.91%) compared to f83afb6
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@carlganz
Copy link
Owner

@carlganz carlganz commented Apr 24, 2018

Merged, thanks for taking time to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.