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

Events calendar view #876

Merged
merged 27 commits into from
Oct 18, 2023
Merged

Conversation

DaanVanVugt
Copy link
Contributor

@DaanVanVugt DaanVanVugt commented Aug 1, 2023

Summary of changes

  • Add a calendar view to the event index page

Motivation and context

It can be a bit annoying to get an overview of what is happening on a specific day/week/month, and people are used to calendars.

Merge after #877

Screenshots

image

separate page:
image

Checklist

  • I have read and followed the CONTRIBUTING guide.
  • I confirm that I have the authority necessary to make this contribution on behalf of its copyright owner and agree
    to license it to the TeSS codebase under the
    BSD license.

@DaanVanVugt DaanVanVugt changed the title Draft: Events calendar view Events calendar view Aug 1, 2023
Copy link
Member

@fbacall fbacall left a comment

Choose a reason for hiding this comment

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

Need some tests also

app/models/concerns/searchable.rb Outdated Show resolved Hide resolved
app/views/events/_compact_event.html.erb Outdated Show resolved Hide resolved
app/controllers/events_controller.rb Outdated Show resolved Hide resolved
app/controllers/events_controller.rb Show resolved Hide resolved
lib/facets.rb Outdated Show resolved Hide resolved
@DaanVanVugt
Copy link
Contributor Author

Do we think the long events need an explicit year? they are very long, after all

Copy link
Member

@fbacall fbacall left a comment

Choose a reason for hiding this comment

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

Sorry for late review, was on holiday.

I tried this out locally and have some comments:

  • The calendar does not seem to load when deep linked (as opposed to clicking the tab), the loader spins endlessly.
  • Does not integrate with the facets: Clicking a facet takes you back to the grid view. We hacked around this for the map view by updating the links in each facet when the map tab is activated. Could possibly do something similar or find a better approach. It should also take you back to the same month you were viewing.
  • The calendar does not work for anonymous users (hence the test failure).
  • The controls to switch to next/previous months could look better - maybe use the same styling as the pagination (with the arrow icons)?

app/views/simple_calendar/_calendar.html.erb Outdated Show resolved Hide resolved
@DaanVanVugt
Copy link
Contributor Author

Sorry for late review, was on holiday.

I tried this out locally and have some comments:

* The calendar does not seem to load when deep linked (as opposed to clicking the tab), the loader spins endlessly.

* Does not integrate with the facets: Clicking a facet takes you back to the grid view. We hacked around this for the map view by updating the links in each facet when the map tab is activated. Could possibly do something similar or find a better approach. It should also take you back to the same month you were viewing.

* The calendar does not work for anonymous users (hence the test failure).

* The controls to switch to next/previous months could look better - maybe use the same styling as the pagination (with the arrow icons)?

All good points, and all fixed. Deeplinking works now, the currently selected month is kept when using facets, tests are fixed etc. One difficulty was with deeplinking to a specific month, but I think we can get by without that for now.

@DaanVanVugt
Copy link
Contributor Author

I see now I need to change the way we query from SOLR, should do that before we can merge this.

Comment on lines +39 to +47
# override @facet_params to get only events relevant for the current month view
@facet_params[:running_during] = "#{Date.today.beginning_of_day}/#{@start_date + 1.month}"
fetch_resources
events_set = @events

@facet_params[:running_during] = "#{@start_date}/#{@start_date + 1.month}"
fetch_resources
events_set += @events

Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to do 2 solr queries?

Copy link
Contributor

@mikesndrs mikesndrs Oct 3, 2023

Choose a reason for hiding this comment

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

Right now it is done to make sure that the relevant events after the current day are not left out in favor of the past events if there are a lot of them. Looking for a better solution

get :index
assert_select 'li a[href=?]', '#calendar', count: 1
get :calendar
assert_text 'relevant_event'
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be a valid method. Maybe you could do something like @response.body.include?

@@ -20,7 +20,9 @@
<% end %>
<% content_for :display_options do %>
<ul class="nav nav-xs nav-pills index-display-options">
<%# We should consider setting the active status based on the query fragment so we can deeplink %>
Copy link
Member

Choose a reason for hiding this comment

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

I was under the impression that the tab history JavaScript did this dynamically after the page loads

<%= link_to event do %>
<%= event.title %>
-
<%= l event.start, format: :long if event.start %> - <%= l event.end, format: :long if event.end %>
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to have something like [l(event.start, format: :long), l(event.end, format: :long)].compact.join(' - ')
to avoid a random - when only one date is present.

Or you could use the neatly_printed_date_range helper method.

@fbacall
Copy link
Member

fbacall commented Oct 13, 2023

@mikesndrs are you making any more changes or should I merge?

@mikesndrs
Copy link
Contributor

Pretty much done but if we ever find a better solution than 2 solr queries we will make a new PR

@fbacall fbacall merged commit 65ff3d2 into ElixirTeSS:master Oct 18, 2023
7 checks passed
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.

3 participants