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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnoneill
Copy link
Contributor

@dnoneill dnoneill commented Oct 21, 2024

Copy link
Contributor

@corylown corylown left a 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| %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<%= 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| %>

Comment on lines +19 to +24
<% 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 %>

Copy link
Contributor

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 %>
Copy link
Contributor

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.

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