-
Notifications
You must be signed in to change notification settings - Fork 5
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
Added filters Country and Provider to Load tab to #164
Conversation
Only the file R/mod_query_data.R is to be merged in. The other 3 files are not supposed to be in the commit, but my .gitignore didn't work correctly. |
@JamesBisese - I can now see these changes - thanks! @wokenny13 and I will review. |
@JamesBisese - I started taking a look at this today and realized I need to update my version of R due to some error messages I'm getting related to jsonlite package. I put in the request for the update today and will resume my review once the update is installed. |
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.
I was able to run the app and review the updates. I've looked into the updates made for the 3 issues, ran through an example dataset, and they all seem good on my end. * Only the file R/mod_query_data.R should be merged and not the other files.
Characteristic Groups: Allows for multiple selections correctly.
Missing Country Code Filter: Country/Ocean filter can be seen and used.
Provider Filter: Filter for NWIS and WQX can be found on the import tab and used.
column( | ||
4, | ||
shiny::checkboxGroupInput(ns("providers"), | ||
"Data Source", |
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.
@JamesBisese - is it possible for the provider filter default setting to be both providers checked? To mimic WQP querying interface. If the user unchecks both boxes, it would be ideal to still query both providers but provide a message that both users are being queried because the user has not made any provider selection(s).
@@ -5,4 +5,4 @@ default: | |||
production: | |||
app_prod: yes | |||
dev: | |||
golem_wd: !expr here::here() | |||
golem_wd: !expr golem::pkg_path() |
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.
@JamesBisese - I can fix/restore the unintentional changes to app.R and golem-config.yml. Once you get a chance to update the provider filter settings, I will merge this pull request. Thanks!
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.
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.
The original check boxes work fine. In a new PR (#165), I made it so both are checked by default:
Updates (should be) only to mod_query_data.R and are fixes for 3 issues - #140 Fix characterisitic group filter on import tab, #146 Import tab missing country code filter, and #129 Add providers as a filter option on import tab.