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

feat: Show "Depart at" buttons and show the correct itinerary on click #2238

Merged
merged 16 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion assets/tailwind.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ module.exports = {
},
"brand-primary": {
DEFAULT: "#165c96",
darkest: "#0b2f4c"
darkest: "#0b2f4c",
lightest: "#cee0f4"
},
"logan-express": {
BB: "#f16823",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ defmodule DotcomWeb.Components.LiveComponents.TripPlannerResultsSection do

@impl true
def mount(socket) do
{:ok, socket |> assign(:expanded_itinerary_index, nil)}
{:ok,
socket
|> assign(:expanded_itinerary_index, nil)
|> assign(:selected_itinerary_detail_index, 0)}
Copy link
Contributor Author

@joshlarson joshlarson Dec 2, 2024

Choose a reason for hiding this comment

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

Per this comment, both of these bits of state are going to get moved up to the TripPlanner level in a follow-up PR.

end

@impl true
Expand Down Expand Up @@ -64,6 +67,7 @@ defmodule DotcomWeb.Components.LiveComponents.TripPlannerResultsSection do
<.itinerary_panel
results={results}
details_index={@expanded_itinerary_index}
selected_itinerary_detail_index={@selected_itinerary_detail_index}
target={@myself}
/>
</div>
Expand Down Expand Up @@ -117,7 +121,11 @@ defmodule DotcomWeb.Components.LiveComponents.TripPlannerResultsSection do
<.itinerary_summary summary={@summary} />
</div>

<.itinerary_detail :for={itinerary <- @itineraries} itinerary={itinerary} />
<.itinerary_detail
itineraries={@itineraries}
selected_itinerary_detail_index={@selected_itinerary_detail_index}
target={@target}
/>
</div>
"""
end
Expand All @@ -144,6 +152,13 @@ defmodule DotcomWeb.Components.LiveComponents.TripPlannerResultsSection do
{:noreply, socket |> assign(:expanded_itinerary_index, index)}
end

@impl true
def handle_event("set_selected_itinerary_detail_index", %{"trip-index" => index_str}, socket) do
{index, ""} = Integer.parse(index_str)

{:noreply, socket |> assign(:selected_itinerary_detail_index, index)}
end

defp format_datetime_short(datetime) do
Timex.format!(datetime, "%-I:%M", :strftime)
end
Expand Down
64 changes: 58 additions & 6 deletions lib/dotcom_web/components/trip_planner/itinerary_detail.ex
Original file line number Diff line number Diff line change
@@ -1,14 +1,66 @@
defmodule DotcomWeb.Components.TripPlanner.ItineraryDetail do
@moduledoc """
A component to render an itinerary in detail..
The section of the trip planner page that shows the map and
the summary or details panel
"""

use DotcomWeb, :component

import DotcomWeb.Components.TripPlanner.Leg

alias Dotcom.TripPlan.PersonalDetail

def itinerary_detail(assigns) do
def itinerary_detail(
%{
itineraries: itineraries,
selected_itinerary_detail_index: selected_itinerary_detail_index
} = assigns
) do
assigns =
assign(assigns, :selected_itinerary, Enum.at(itineraries, selected_itinerary_detail_index))

~H"""
<div>
<p class="text-sm mb-2 mt-3">Depart at</p>
<div class="flex">
<.depart_at_button
:for={{itinerary, index} <- Enum.with_index(@itineraries)}
active={@selected_itinerary_detail_index == index}
phx-click="set_selected_itinerary_detail_index"
phx-value-trip-index={index}
phx-target={@target}
>
<%= Timex.format!(itinerary.start, "%-I:%M%p", :strftime) %>
</.depart_at_button>
</div>
<.specific_itinerary_detail itinerary={@selected_itinerary} />
</div>
"""
end

attr :active, :boolean
attr :rest, :global
slot :inner_block

defp depart_at_button(%{active: active} = assigns) do
background_class = if active, do: "bg-brand-primary-lightest", else: "bg-transparent"
assigns = assign(assigns, :background_class, background_class)

~H"""
<button
type="button"
class={[
"border border-brand-primary rounded px-2.5 py-1.5 mr-2 text-brand-primary text-lg",
"hover:bg-brand-primary-lightest #{@background_class}"
]}
{@rest}
>
<%= render_slot(@inner_block) %>
</button>
"""
end

defp specific_itinerary_detail(assigns) do
assigns =
assign(
assigns,
Expand All @@ -19,11 +71,11 @@ defmodule DotcomWeb.Components.TripPlanner.ItineraryDetail do
)

~H"""
<details class="mt-4">
<summary class="border border-2 border-brand-primary px-3 py-2 flex items-center hover:border-brand-primary-darkest hover:bg-gray-lighter">
<div class="mt-4">
<div>
Depart at <%= Timex.format!(@itinerary.start, "%-I:%M%p", :strftime) %>
<.route_symbol :for={route <- @all_routes} route={route} class="ml-2" />
</summary>
</div>
<div :for={leg <- @itinerary.legs}>
<.leg
start_time={leg.start}
Expand All @@ -35,7 +87,7 @@ defmodule DotcomWeb.Components.TripPlanner.ItineraryDetail do
realtime_state={leg.realtime_state}
/>
</div>
</details>
</div>
"""
end
end
90 changes: 77 additions & 13 deletions test/dotcom_web/live/trip_planner_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,39 @@ defmodule DotcomWeb.Live.TripPlannerTest do

import Mox
import Phoenix.LiveViewTest
import Test.Support.Factories.TripPlanner.TripPlanner, only: [limit_route_types: 1]

alias OpenTripPlannerClient.Test.Support.Factory, as: OtpFactory

setup :verify_on_exit!

defp stub_otp_results(itineraries) do
expect(OpenTripPlannerClient.Mock, :plan, fn _ ->
{:ok, %OpenTripPlannerClient.Plan{itineraries: itineraries}}
end)
end

defp stub_populated_otp_results() do
# Uhhh the OTP factory will generate with any route_type value but our
# parsing will break with unexpected route types
itineraries =
OtpFactory.build_list(3, :itinerary)
|> Enum.map(&limit_route_types/1)

stub_otp_results(itineraries)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we already do this in a factory?


If not, we should probably fix the factory rather than add that code here (since it applies to all code that uses the OtpFactory and not just this test).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do this in a factory (I didn't add this - I just moved it), and I do think we should. I was thinking that fixing that could be a follow-up PR, so that the depart-at buttons can still go-live.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine as long as we do it later. I know the factory does this, but it also parses it. Might be worth trying to use the factory directly:

end

defp update_trip_details(itinerary, trip_id: trip_id, start_time: start_time) do
updated_transit_leg =
itinerary.legs
|> Enum.at(1)
|> update_in([:trip, :gtfs_id], fn _ -> "ma_mbta_us:#{trip_id}" end)

itinerary
|> Map.update!(:legs, &List.replace_at(&1, 1, updated_transit_leg))
|> Map.put(:start, DateTime.new!(Date.utc_today(), start_time, "America/New_York"))
end

test "Preview version behind basic auth", %{conn: conn} do
conn = get(conn, ~p"/preview/trip-planner")

Expand Down Expand Up @@ -96,9 +126,7 @@ defmodule DotcomWeb.Live.TripPlannerTest do
}
}

expect(OpenTripPlannerClient.Mock, :plan, fn _ ->
{:ok, %OpenTripPlannerClient.Plan{itineraries: []}}
end)
stub_otp_results([])
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're only using this in one place, I would just write this as an expectation in this test. That would allow you to remove one of the two functions above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stub_otp_results is called in two places - once here and once in stub_populated_otp_results. I guess I could inline it in both places, but I wanted to avoid duplicating 👇

expect(OpenTripPlannerClient.Mock, :plan, fn _ ->
  {:ok, %OpenTripPlannerClient.Plan{itineraries: itineraries}}
end)


{:ok, view, _html} = live(conn, ~p"/preview/trip-planner?#{params}")

Expand All @@ -115,16 +143,6 @@ defmodule DotcomWeb.Live.TripPlannerTest do
Test.Support.Factories.Stops.Stop.build(:stop)
end)

# Uhhh the OTP factory will generate with any route_type value but our
# parsing will break with unexpected route types
itineraries =
OpenTripPlannerClient.Test.Support.Factory.build_list(3, :itinerary)
|> Enum.map(&Test.Support.Factories.TripPlanner.TripPlanner.limit_route_types/1)

expect(OpenTripPlannerClient.Mock, :plan, fn _ ->
{:ok, %OpenTripPlannerClient.Plan{itineraries: itineraries}}
end)

%{
conn:
put_req_header(
Expand All @@ -144,12 +162,16 @@ defmodule DotcomWeb.Live.TripPlannerTest do
end

test "starts out with no 'View All Options' button", %{conn: conn, params: params} do
stub_populated_otp_results()

{:ok, view, _html} = live(conn, ~p"/preview/trip-planner?#{params}")

refute render_async(view) =~ "View All Options"
end

test "clicking 'Details' button opens details view", %{conn: conn, params: params} do
stub_populated_otp_results()

{:ok, view, _html} = live(conn, ~p"/preview/trip-planner?#{params}")

render_async(view)
Expand All @@ -162,6 +184,8 @@ defmodule DotcomWeb.Live.TripPlannerTest do
conn: conn,
params: params
} do
stub_populated_otp_results()

{:ok, view, _html} = live(conn, ~p"/preview/trip-planner?#{params}")

render_async(view)
Expand All @@ -171,5 +195,45 @@ defmodule DotcomWeb.Live.TripPlannerTest do

refute render_async(view) =~ "View All Options"
end

test "'Depart At' buttons toggle which itinerary to show", %{
conn: conn,
params: params
} do
trip_time_1 = ~T[09:26:00]
trip_id_1 = "trip_id_1"

trip_time_2 = ~T[10:46:00]
trip_time_display_2 = "10:46AM"
trip_id_2 = "trip_id_2"

base_itinerary =
OtpFactory.build(:itinerary)
|> limit_route_types()

# Right now, the headsign (which is what we actually want to
# show) is not available from OTP client, but we're rendering
# the trip ID in its place. Once the headsign is available, we
# should update these updates and the assertions below to use
# the headsign instead of the trip ID.
stub_otp_results([
update_trip_details(base_itinerary, trip_id: "trip_id_1", start_time: trip_time_1),
update_trip_details(base_itinerary, trip_id: "trip_id_2", start_time: trip_time_2)
])

{:ok, view, _html} = live(conn, ~p"/preview/trip-planner?#{params}")

render_async(view)

view |> element("button", "Details") |> render_click()

assert render_async(view) =~ trip_id_1
refute render_async(view) =~ trip_id_2

view |> element("button", trip_time_display_2) |> render_click()

assert render_async(view) =~ trip_id_2
refute render_async(view) =~ trip_id_1
end
end
end
Loading