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

Use Repo.teasers for event index #85

Merged
merged 7 commits into from
Jul 16, 2019
Merged

Use Repo.teasers for event index #85

merged 7 commits into from
Jul 16, 2019

Conversation

amaisano
Copy link
Contributor

@amaisano amaisano commented Jul 6, 2019

Summary of changes

Asana Ticket: Use teaser request for Events

@amaisano amaisano force-pushed the agm-event-teasers branch 3 times, most recently from 2a373f1 to 64c7215 Compare July 7, 2019 02:35
@amaisano amaisano changed the base branch from master to agm-teaser-end-time July 8, 2019 13:08
@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #85 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   96.76%   96.77%   +<.01%     
==========================================
  Files         346      346              
  Lines        5446     5449       +3     
==========================================
+ Hits         5270     5273       +3     
  Misses        176      176

Copy link
Contributor

@ryan-mahoney ryan-mahoney left a comment

Choose a reason for hiding this comment

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

a few suggestions / questions

sort_order: "ASC"
)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit repetitive, how do you feel about:

def get_events_async(id, timeframe) do
    fn ->
      Repo.teasers(
        type: :event,
        related_to: id,
        items_per_page: 10,
        date_op: if timeframe == :past, do: "<", else: ">=",
        date: [value: "now"],
        sort_order: if timeframe == :past, do: "DESC" else: "ASC"
      )
    end
  end

%Content.Teaser{path: path} -> fn -> cms_static_page_path(@conn, path) end
%Content.Event{} -> fn -> event_path(@conn, :show, event) end
end
%>
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't read too deeply into this, but I though that the list would always be Teaser.t() and not Event.t()?

Copy link
Contributor Author

@amaisano amaisano Jul 11, 2019

Choose a reason for hiding this comment

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

We're keeping support for when this template is used by the very old (and not often used) Upcoming Board Meetings paragraph, which serves us Event.t() structs. See this commit for a little more detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amaisano amaisano changed the base branch from agm-teaser-end-time to master July 16, 2019 15:59
- make event times naive once parsed to avoid double TZ shift
- we only have the start time at the moment, not the whole range
- event list is shared by multiple source functions, one of which is still providing %Event{} structs and is out of scope for changing at the moment
@amaisano
Copy link
Contributor Author

@ryan-mahoney now that the dependent branch is merged into master I changed the base of this. I think you already approved it via comment but now I need an official approval (pending tests). Thanks.

@amaisano amaisano merged commit 7bf2cdc into master Jul 16, 2019
@amaisano amaisano deleted the agm-event-teasers branch July 16, 2019 16:52
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