Skip to content

Prep quickstart #2

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

Merged
merged 61 commits into from
Mar 1, 2019
Merged

Prep quickstart #2

merged 61 commits into from
Mar 1, 2019

Conversation

colearendt
Copy link
Contributor

@colearendt colearendt commented Feb 2, 2019

A handful of changes to look over:

  • add manifest.json, .connect.yml and .internal.yml (b/c the QuickStart bash script is behind the R package at present) to support programmatic deployment
  • controversial: add a config.yml that pivots how image-classifier behaves (you can choose download or upload)
  • add default images under ./image-classifier/img so that those can be used without the internet
  • fix image-classifier requirements.txt
  • for some reason renderImage deletes the file off of disk... so I added a copy to temp space using the fs package
  • add height = NULL on the image render to protect against a weird wrapping issue for large images
  • add width: 100% on the image to fix another weird wrapping issue for large images

Also a handful of tidbits / tips / tricks:

  • using observeEvent for button handling is usually the preferred pattern (and it is pretty concise 😄 )
  • Check into reactiveVal if you haven't!! It is probably one of my favorites! I used this to abstract image_path away
  • You rarely ever want to call stop in a Shiny app, in any context. The reason is because it kicks the user off of their session , which means any user who is on the application (think Connect, where many users use the same R session) get kicked off (i.e. not just the one who generated the error) . (Oops. My goof - it doesn't kick off all users. But you can if you want 😉 Check out this one, and open it up in a few browser windows). Usually it is only for terminal errors (like bad configuration at startup).
  • If you want to expose the error to users, you can use showNotification() (although usually you would not want to just expose arbitrary errors and say something more generic... like "WARNING: Download failed")
  • renderUI/uiOutput is another trick to keep up your sleeve for rendering UI elements dynamically

@koverholt
Copy link
Contributor

This all LGTM when you are ready to merge. The note about tweaking fetch_image_url is up to you, feel free to use whatever makes your life easier. Thanks!

@colearendt
Copy link
Contributor Author

@koverholt I know I made a bunch more changes since you approved this one - are you still good for me to merge this? This is the branch I used in the latest QuickStart

@koverholt
Copy link
Contributor

Yep, I looked through the changes (and have also tested these in the QuickStart), and it all LGTM.

@colearendt colearendt merged commit adb2d4b into master Mar 1, 2019
@colearendt colearendt deleted the prep-quickstart branch March 1, 2019 03:04
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