Closed brentscott93 closed 4 years ago
Thanks Brent! I haven't done a very deep dive into the code but based on a quick skim there are two things I would want to change:
Seems a bit esoteric to have height and race types. I'm not a big fan of adding functionality that's very specific to a general purpose tool.
Since you're setting options(scipen=999)
you need to re-set this option to what it was when the app exists. We don't want to cause side effects to the user's environment just from running the app.
And a few other comments that I'll leave it up to you if you want to address:
Is shinydashboard necessary? The UI looks very nice but doesn't look like it uses much from shinydashboard. Is it only using the box
UI from it? If so, could that be replicated without importing the package and making the entire UI use shinydashboard?
For defensive programming reasons, I would not use question$id
and question$type
but instead question[["id"]]
and question[["type"]]
because of R's partial matching. In interactive scripts or personal projects it's fine to use partial matching syntax, but in a package meant to be used by others and robust, it's safer to avoid that
Whitespace in the coding style is inconsistent (sometimes if(x){
and sometimes if (x) {
)
Use TRUE
and FALSE
rather than T
and F
For argument checking, using a package such as {checkmate} (or others) if more robust than using an if statement that just checks a single condition. For example see here
(Some of these points may seem hypocritical because the style in the current codebase isn't great, but that's because it was done in a hackathon and moving forward I don't want to add more bad style)
Thanks for the feedback and I understand the rationale behind the critiques (well received).
options()
.box()
function. I was able to remove the {shinydashboard} dependency by using CSS to style the looks of AdminLTE directly.$
. Indexing with [[
now. The updates have been applied, committed, and re-published a demo app.
Thanks for the time you are taking to review this and for the consideration.
Looks great!
FYI I tend to use [[
over $
in some cases, but not always, mostly when there's user-provided content. For example, the input
list is something that the user can't change, you as the developer should be sure of what it contains, so I use input$
. But something like the question list, because you're relying on the user to construct that list, that's why I suggested using [[
, because you want to be sure they typed the right thing.
After making changes, when I re-reread the comments I realized you might have meant that...ha! Thanks for clarifying and for all the feedback/tips.
Hi Dean -
This PR is a proposal to add additional functionality to {shinyforms}. There are no changes to any existing functions or code that is currently implemented. There is an addition of one exported function (shinyforms::quickform) and several un-exported helper functions.
Key differences:
Design Philosophy Offers quick development and deployment of a single file shiny app with limited flexibility.
Limitations
Demo app here