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.

Sign up for GitHub

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

Jump to bottom

Refactor: merge the plotting modules into one single module #30

Closed
milanmlft opened this issue Aug 19, 2024 · 1 comment · Fixed by #46
Closed

Refactor: merge the plotting modules into one single module #30

milanmlft opened this issue Aug 19, 2024 · 1 comment · Fixed by #46
Assignees
@milanmlft
Labels
improvement

Comments

@milanmlft
Copy link
Member

milanmlft commented Aug 19, 2024 •
edited
Loading

Definition of Done / Acceptance Criteria

We have one single Shiny module that handles the generation of the summary plots for the dashboard.

Testing

No response

Documentation

No response

Dependencies

No response

Details and Comments

Currently, there are mod_monthly_count and mod_stat_numeric, but these do largely the same. So in order to make the code more DRY we could have a single mod_plots module with a server function that handles the different plot types. In the main app_server we would then have something like:

mod_plots_server(data = monthly_counts, type = "monthly_counts")
mod_plots_server(data = summary_stats, type = "summary_stat_numeric")
mod_plots_server(data = summary_stats, type = "summary_stat_categorical")
@linear Linear
Copy link

linear bot commented Aug 19, 2024

SAF-646 Refactor: merge the plotting modules into one single module

Definition of Done / Acceptance Criteria

We have one single Shiny module that handles the generation of the summary plots for the dashboard.

Testing

No response

Documentation

No response

Dependencies

No response

Details and Comments

Currently, there is mod_monthly_count

@milanmlft milanmlft added the improvement label Aug 19, 2024 — with Linear
milanmlft added a commit that referenced this issue Aug 29, 2024
@milanmlft
Make plotting module generic for the type of plot
4335593 
Fixes #30
@milanmlft milanmlft mentioned this issue Aug 30, 2024
Merged
@milanmlft milanmlft self-assigned this Sep 2, 2024
milanmlft added a commit that referenced this issue Sep 4, 2024
@milanmlft
Fix bug in app startup and refactor (#46)
872e364 
* Only check envvars when running in prod

* Update tests: module servers should fail on missing inputs

* Fix: get rid of warning/error at app startup when no concept is selected yet

Use `shiny::req()` to simplify checking for existence of reactive inputs

* Refactor: use single module for dashboard plots

* Refactor: update UI elements for plots

* Make plotting module generic for the type of plot

Fixes #30

* Move common module server tests to `test-mod_plots.R`

* Rename test files, they now only only contain business-logic tests

* Remove old modules

* Update doc

* Pass linter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@milanmlft milanmlft

Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix bug in app startup and refactor SAFEHR-data/omopcat
1 participant
@milanmlft