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 quickform() function #45

Merged
merged 24 commits into from
Nov 18, 2020
Merged

Add quickform() function #45

merged 24 commits into from
Nov 18, 2020

Conversation

brentscott93
Copy link
Contributor

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:

  • Implemented with {shinydashboard} to look as much like a Google Form as possible.
  • Allow users to return to survey to edit or update responses by generating unique ID #s that can be optionally emailed to end-user.
  • Minimal coding/knowledge of shiny (no modules - maybe similar to enhancement #9)
  • Standalone app.R. quickform() is basically a wrapper round shinyApp(ui, server).

Design Philosophy
Offers quick development and deployment of a single file shiny app with limited flexibility.

Limitations

  • Only supports storage on Google Drive using {googledrive} and {googlesheets4} (or local storage).
  • Adds dependencies

Demo app here

@daattali
Copy link
Owner

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)

@brentscott93
Copy link
Contributor Author

brentscott93 commented Nov 16, 2020

Thanks for the feedback and I understand the rationale behind the critiques (well received).

  • The height/race question types have been removed.
  • The function no longer needs to set options().
  • You are correct in that {shinydashboard} was only being used for the box() function. I was able to remove the {shinydashboard} dependency by using CSS to style the looks of AdminLTE directly.
  • Added {checkmate} for argument checking (never heard of this one - good tip!)
  • There were a lot of inconsistency in styling so I tried to be more careful with whitespace, abbreviations, etc.
  • Thanks for the reminder on partial matching with $. 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.

@daattali
Copy link
Owner

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.

@daattali daattali merged commit a938ab0 into daattali:master Nov 18, 2020
@brentscott93
Copy link
Contributor Author

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.

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