-
Notifications
You must be signed in to change notification settings - Fork 16
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
Demo app for Dash to connect to PostgreSQL #259
Conversation
@bchen39 Please deploy the app send the link to make it easier for us to test. Let me know if you still need your dev account upgraded |
link to app, let me know if this works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bchen39 It looks like it's down, but it was working before. If you didn't delete it, it might have gotten cleaned up automatically.
A few notes from brief testing:
- Add a paragraph under the title to provide some context for each of the datasets. It's hard to understand what each one describes
- Some of the columns don't make sense without context (Walc, etc.). Might be clearer to re-phrase these or just delete them
- Changing the axes via the dropdown didn't reload the graphs (couldn't verify since the app went down)
Only keep readable data and added more detailed description.
@bryannho Please check my latest version. I made sure the title of each graph explains what they are, and cleaned up data that do not make sense. The app should work properly now. Let me know in case the app gets cleaned up again and I can redeploy. |
@bchen39 let's use the prod environment from now on for the demo apps |
@bchen39 it got cleaned up, please deploy on prod and resend the link. @edublancas I think he'll need a pro account since it's a docker app, can he use the [email protected] one? |
@bryannho link to app on prod environment. This should work now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments regarding secrets. ensure to mention this in the blog post: that people should use our secrets feature
also, since the db credentials have been disclosed publicly, ensure you change the db authentication details
Added more detailed steps on how to upload dataset and configure secret variables in README
@bryannho please help me checking the comments from my existing review and approve if you think it's good to go now |
Please try deploying the app using the |
Additionally: removed Dockerfile and removed extra package from requirements.txt
|
Is this ready for review? @bchen39 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few more comments
Modified local testing instruction in README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one minor comment
dismissing to merge
Added example Dash app that is connected to Postgres database.
Closes ploomber.io#186
📚 Documentation preview 📚: https://ploomber-doc--259.org.readthedocs.build/en/259/