-
Notifications
You must be signed in to change notification settings - Fork 14
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
Events calendar view #876
Conversation
99c7e69
to
5a542a0
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.
Need some tests also
c08d30e
to
23b8e7d
Compare
Do we think the long events need an explicit year? they are very long, after all |
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.
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. |
I see now I need to change the way we query from SOLR, should do that before we can merge this. |
# 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 | ||
|
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.
Is it necessary to do 2 solr queries?
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.
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' |
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.
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 %> |
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 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 %> |
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.
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.
@mikesndrs are you making any more changes or should I merge? |
Pretty much done but if we ever find a better solution than 2 solr queries we will make a new PR |
Summary of changes
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
separate page:
Checklist
to license it to the TeSS codebase under the
BSD license.