-
Notifications
You must be signed in to change notification settings - Fork 64
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 date selector to analytics dashboard #3222
base: main
Are you sure you want to change the base?
Conversation
2d3f943
to
344bd29
Compare
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.
It would be nice to add some checks for acceptable parameters (especially since this displays the supplied parameters to the page) before submitting the request to Google Analytics, with helpful feedback in a flash message about supplying a valid date (or handle the error from GA and then display a helpful flash error). Currently if you enter a bogus date or other garbage in the parameters this gets sent to the GA API and you'll get an application error.
<p><%= note %></p> | ||
<% end %> | ||
|
||
<%= form_with url: request.path, class: 'mb-3 d-flex', method: :get do |form| %> |
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.
<%= form_with url: request.path, class: 'mb-3 d-flex', method: :get do |form| %> | |
<%= form_with url: helpers.analytics_exhibit_dashboard_path(@current_exhibit), class: 'mb-3 d-flex', method: :get do |form| %> |
<% if dynamic_heading? %> | ||
<h2><%= I18n.t("spotlight.dashboards.analytics.reporting_period_heading_dynamic", start_date: dates['start_date'], end_date: dates['end_date']) %></h2> | ||
<% else %> | ||
<h2><%= I18n.t("spotlight.dashboards.analytics.reporting_period_heading") %></h2> | ||
<% end %> | ||
|
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.
Would it make sense to move this logic to the component class instead? You could have a heading method there that returns the right translation depending on if the date parameters are set.
<%= form.submit I18n.t("spotlight.dashboards.analytics.submit_date_label"), class: 'btn btn-primary' %> | ||
</div> | ||
<% end %> | ||
|
||
<% if results? %> | ||
<%= cache current_exhibit, expires_in: 1.hour do %> |
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.
With caching turned on in development the first request to the page gets cached and then none of the date range parameters I apply get reflected on the page. I think you need to change the cache key to account for any parameters applied.
part of sul-dlss/exhibits#2612