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

Usage of dynamic dots in router_ui #126

Merged
merged 8 commits into from
Apr 11, 2023
Merged

Conversation

vanhry
Copy link
Contributor

@vanhry vanhry commented Mar 29, 2023

Changes

  • Change dots ... argument in router_ui to allow dynamically pass arguments
  • According test was added
  • One example of usage was added

@vanhry vanhry changed the title [WIP] Usage of dynamic dots in router_ui Usage of dynamic dots in router_ui Mar 30, 2023
Copy link
Member

@jakubnowicki jakubnowicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is a nice improvement to the package. Some changes are required, but rather minor ones. Also, please update the package version in the DESCRIPTION.

R/router.R Outdated Show resolved Hide resolved
R/router.R Outdated Show resolved Hide resolved
R/router.R Outdated
Comment on lines 185 to 187
#' @param ... <[`dynamic-dots`][rlang::dyn-dots]>
#' All other routes defined with shiny.router::route function.
#' It's possible to pass routes in dynamic way with dynamic dots
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the beginning (<[dynamic-dots][rlang::dyn-dots]>). Instead, after mentioning the possibility to use dynamic dots please:

  • provide a link to rlang documentation with dynamic dots explanation
  • mention the example

Copy link
Contributor Author

@vanhry vanhry Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, should I somehow add the link to the file in examples/?

R/router.R Show resolved Hide resolved
R/router.R Show resolved Hide resolved
inst/WORDLIST Outdated Show resolved Hide resolved
tests/testthat/test-router.R Outdated Show resolved Hide resolved
tests/testthat/test-router.R Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
Copy link
Member

@jakubnowicki jakubnowicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jakubnowicki jakubnowicki merged commit ca630c5 into main Apr 11, 2023
@jakubnowicki jakubnowicki deleted the dynamic_dots.router_ui branch April 11, 2023 14:16
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.

2 participants