Subway status in alerts page display #2388
Conversation
There was a problem hiding this comment.
This looks so good! I love how simple the solution is, and how clearly it exposes how the original version of this can be improved even more! 😅
I added some non-blocking thoughts. The two that are blocking are:
- I'm pretty sure the type in
Subway.subway_statusshould be a list, rather than a single alert (I guess Dialyzer will also block approval on that 😅). The way the alerts are rendering doesn't look quite right.
EDIT: Just realized from looking in Slack that the alert rendering is already on your radar, and blocked by another PR 🤦♂️
| defp status_to_alerts_rows(subway_status) do | ||
| @route_ids | ||
| |> Enum.map(&{&1, subway_status |> Map.get(&1)}) | ||
| |> Enum.flat_map(&rows_for_route(&1, false)) |
There was a problem hiding this comment.
Suggestion (non-blocking): Use a keyword argument to make it clearer what the boolean value that's being passed in is there for.
| |> Enum.flat_map(&rows_for_route(&1, false)) | |
| |> Enum.flat_map(&rows_for_route(&1, collapse: false)) |
Another option would be finding a different way of accomplishing the combining, for instance, pushing the combine-like-alerts step to a homepage-only post-processing step (kind of like the other collapsing bits), but honestly I don't think that's necessary for this PR.
There was a problem hiding this comment.
Oh, I like this!
| @@ -87,27 +61,31 @@ defmodule DotcomWeb.Live.SystemStatus do | |||
| """ | |||
| end | |||
|
|
|||
| defp alert(assigns) do | |||
| defp example_table(assigns) do | |||
There was a problem hiding this comment.
This is such a nice presentation for this!
| </div> | ||
| </:heading> | ||
| <:content> | ||
| {Phoenix.View.render_one(row.alert, DotcomWeb.AlertView, "_item.html", |
There was a problem hiding this comment.
Annoyingly I think we'll need a component here that's slightly different from the regular alert, because this creates a kind of accordion-within-an-accordion situation, which is different from what's in the designs.
Figma
Current branch
There was a problem hiding this comment.
Yeah, the actual plan is to use the alert component from Anthony's branch in #2387
Co-authored-by: Josh Larson <[email protected]>
lib/dotcom/system_status.ex
Outdated
| |> SystemStatus.Alerts.filter_relevant() | ||
| Dotcom.Alerts.Disruptions.Subway.todays_disruptions() | ||
| |> Map.get(:today, []) | ||
| |> SystemStatus.Alerts.for_day(@date_time_module.now()) |
There was a problem hiding this comment.
I removed the underlying defp @joshlarson had here... what's it for? I can't tell if I should've kept it?
There was a problem hiding this comment.
I don't think you'll need that extra call. Is there a difference between:
Dotcom.Alerts.Disruptions.Subway.todays_disruptions()
|> Map.get(:today, [])
and
Dotcom.Alerts.Disruptions.Subway.todays_disruptions()
|> Map.get(:today, [])
|> SystemStatus.Alerts.for_day(@date_time_module.now())
There was a problem hiding this comment.
Maybe if it just filters out alerts that are today, but earlier today, that can just be a private function in this module.
| @@ -36,7 +37,7 @@ defmodule Dotcom.Alerts.Disruptions.Subway do | |||
| # 4. Sorts the alerts within the group by start time. | |||
| defp disruption_groups() do | |||
| subway_route_ids() | |||
| |> @alerts_repo.by_route_ids(Utils.DateTime.now()) | |||
| |> @alerts_repo.by_route_ids(@date_time_module.now()) | |||
There was a problem hiding this comment.
I was very amused we'd forgotten this!
lib/dotcom/system_status/alerts.ex
Outdated
| def active_now_or_later_on_day?(alert, datetime) do | ||
| Enum.any?(alert.active_period, fn {active_period_start, active_period_end} -> | ||
| starts_before_end_of_service?(active_period_start, datetime) && | ||
| has_not_ended?(active_period_end, datetime) | ||
| end) | ||
| end | ||
|
|
||
| @doc """ | ||
| Given a list of alerts, filters only the ones that are active today, as defined in `active_on_day?/2`. | ||
| Given a list of alerts, filters only the ones that are active at the, as defined in `active_now_or_later_on_day?/2`. | ||
| """ | ||
| def for_day(alerts, datetime) do | ||
| Enum.filter(alerts, &active_on_day?(&1, datetime)) | ||
| Enum.filter(alerts, &active_now_or_later_on_day?(&1, datetime)) |
There was a problem hiding this comment.
Unless these are used anywhere else, I think they can be moved to the system status module as a private function.
also fix it a bit since the struct's default "" are truthy
Summary of changes
Asana Ticket: Service Alerts page | Add "Current Status" section for subway
The component's got a few differences to its homepage counterpart! There's a lot of silly code things I'm working around so this implementation is on the slightly "dumb" side, but it works while building off the existing logic.
The preview page is modified to show both component versions side by side.
Note: this PR uses the existing alert component for now, will be modified to use the same alert format from the planned work widget in #2387