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: Toggle between summary and details view in itinerary panel #2232

Merged
merged 14 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
defmodule DotcomWeb.Components.LiveComponents.TripPlannerResultsSection do
@moduledoc """
The section of the trip planner page that shows the map and
the summary or details panel
"""

use DotcomWeb, :live_component

import DotcomWeb.Components.TripPlanner.ItineraryDetail
import DotcomWeb.Components.TripPlanner.ItineraryGroup, only: [itinerary_group: 1]

@impl true
def mount(socket) do
{:ok, socket |> assign(:expanded_itinerary_index, nil)}
end

@impl true
def render(assigns) do
~H"""
<section class="flex w-full border border-solid border-slate-400">
<div :if={@error} class="w-full p-4 text-rose-400">
<%= inspect(@error) %>
</div>
<.async_result :let={results} assign={@results}>
<div :if={results} class="w-full p-4">
<.itinerary_panel
results={results}
details_index={@expanded_itinerary_index}
target={@myself}
/>
</div>
</.async_result>
<.live_component
module={MbtaMetro.Live.Map}
id="trip-planner-map"
class="h-96 w-full relative overflow-none"
config={@map_config}
pins={[@from, @to]}
/>
</section>
"""
end

defp itinerary_panel(%{details_index: nil} = assigns) do
~H"""
<.itinerary_group
:for={{result, index} <- Enum.with_index(@results)}
index={index}
details_click_event="set_expanded_itinerary_index"
target={@target}
{result}
/>
"""
end

defp itinerary_panel(%{results: results, details_index: details_index} = assigns) do
assigns =
assign(assigns, :itineraries, results |> Enum.at(details_index) |> Map.get(:itineraries))

~H"""
<div class="mt-30">
<button
type="button"
phx-click="set_expanded_itinerary_index"
phx-value-index="nil"
phx-target={@target}
class="btn-link"
>
<p class="flex flex-row items-center">
<.icon class="fill-brand-primary h-4 mr-2" name="chevron-left" />
<span class="font-medium">View All Options</span>
</p>
</button>
<.itinerary_detail :for={itinerary <- @itineraries} itinerary={itinerary} />
</div>
"""
end

@impl true
def handle_event("set_expanded_itinerary_index", %{"index" => index_str}, socket) do
index =
case Integer.parse(index_str) do
{index, ""} -> index
_ -> nil
end

{:noreply, socket |> assign(:expanded_itinerary_index, index)}
end
end
26 changes: 17 additions & 9 deletions lib/dotcom_web/components/trip_planner/itinerary_group.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,24 @@ defmodule DotcomWeb.Components.TripPlanner.ItineraryGroup do
"""
use DotcomWeb, :component

import DotcomWeb.Components.TripPlanner.ItineraryDetail

attr(:summary, :map, doc: "ItineraryGroups.summary()", required: true)
attr(:itineraries, :list, doc: "List of %Dotcom.TripPlan.Itinerary{}", required: true)

attr(:index, :integer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It took me a while to understand what this was for... ultimately I'd favor a more descriptive attribute name, maybe :group_index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking for a moment... the only reason an itinerary group cares about its index is because it does the right thing in response to clicking on Details. So maybe the thing to do is have setting index correctly be handled in a callback instead of implicitly through this param.

I think that would require converting itinerary_group to a live component though, so if we think that's a good idea, my vote would be to do that as a follow-up PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that would work fine (and agree that might need a live component), but it might be overkill?

An alternate thought: the summary and itineraries here are populated by the output of Dotcom.TripPlan.ItineraryGroups.from_itineraries/1, which outputs a list of %{itineraries: itineraries, summary: summary} maps. Why not have this function compute the indexes and have the map include that index as well?

A more meta thought - we're using index to really mean "unique identifier". The past iteration made up a unique ID (assign(assigns, :group_id, "group-#{:erlang.phash2(assigns.itineraries)}")). We might want to reconsider using the index value for that purpose (what if we want to let end users re-sort the list later, for example).

doc: "Index into the full list where this itinerary group sits",
required: true
)

attr :target, :string, doc: "The target that should receive events", required: true

attr :details_click_event, :string,
doc: "The event that fires when 'Details' is clicked",
required: true

@doc """
Renders a single itinerary group.
"""
def itinerary_group(assigns) do
assigns =
assign(assigns, :group_id, "group-#{:erlang.phash2(assigns.itineraries)}")

~H"""
<div class="border border-solid m-4 p-4">
<div
Expand Down Expand Up @@ -60,13 +66,15 @@ defmodule DotcomWeb.Components.TripPlanner.ItineraryGroup do
Similar trips depart at <%= Enum.map(@summary.next_starts, &format_datetime_short/1)
|> Enum.join(", ") %>
</div>
<button class="btn-link font-semibold underline" phx-click={JS.toggle(to: "##{@group_id}")}>
<button
class="btn-link font-semibold underline"
phx-click={@details_click_event}
phx-target={@target}
phx-value-index={@index}
>
Details
</button>
</div>
<div id={@group_id} class="mt-30" style="display: none;">
<.itinerary_detail :for={itinerary <- @itineraries} itinerary={itinerary} />
</div>
</div>
"""
end
Expand Down
30 changes: 11 additions & 19 deletions lib/dotcom_web/live/trip_planner.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@ defmodule DotcomWeb.Live.TripPlanner do

use DotcomWeb, :live_view

import DotcomWeb.Components.TripPlanner.ItineraryGroup, only: [itinerary_group: 1]
import MbtaMetro.Components.{Feedback, Spinner}

alias DotcomWeb.Components.LiveComponents.TripPlannerForm
alias DotcomWeb.Components.LiveComponents.{TripPlannerForm, TripPlannerResultsSection}
alias Dotcom.TripPlan.{AntiCorruptionLayer, InputForm.Modes, ItineraryGroups}

@form_id "trip-planner-form"
Expand Down Expand Up @@ -67,23 +66,16 @@ defmodule DotcomWeb.Live.TripPlanner do
<% end %>
</.async_result>
</section>
<section class="flex w-full border border-solid border-slate-400">
<div :if={@error} class="w-full p-4 text-rose-400">
<%= inspect(@error) %>
</div>
<.async_result :let={results} assign={@results}>
<div :if={results} class="w-full p-4">
<.itinerary_group :for={result <- results} {result} />
</div>
</.async_result>
<.live_component
module={MbtaMetro.Live.Map}
id="trip-planner-map"
class="h-96 w-full relative overflow-none"
config={@map_config}
pins={[@from, @to]}
/>
</section>

<.live_component
module={TripPlannerResultsSection}
id="trip-planner-results"
results={@results}
error={@error}
map_config={@map_config}
from={@from}
to={@to}
/>
</div>
"""
end
Expand Down
105 changes: 105 additions & 0 deletions test/dotcom_web/live/trip_planner_test.exs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
defmodule DotcomWeb.Live.TripPlannerTest do
use DotcomWeb.ConnCase, async: true

import Mox
import Phoenix.LiveViewTest

setup :verify_on_exit!

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

Expand Down Expand Up @@ -67,4 +70,106 @@ defmodule DotcomWeb.Live.TripPlannerTest do
# test "pushes updated location to the map", %{view: view} do
# end
end

describe "Trip Planner with no results" do
setup %{conn: conn} do
[username: username, password: password] =
Application.get_env(:dotcom, DotcomWeb.Router)[:basic_auth]

%{
conn:
put_req_header(
conn,
"authorization",
"Basic " <> Base.encode64("#{username}:#{password}")
)
}
end

test "shows 'No trips found' text", %{conn: conn} do
params = %{
"plan" => %{
"from_latitude" => "#{Faker.Address.latitude()}",
"from_longitude" => "#{Faker.Address.longitude()}",
"to_latitude" => "#{Faker.Address.latitude()}",
"to_longitude" => "#{Faker.Address.longitude()}"
}
}

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

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

assert render_async(view) =~ "No trips found"
end
end

describe "Trip Planner with results" do
setup %{conn: conn} do
[username: username, password: password] =
Application.get_env(:dotcom, DotcomWeb.Router)[:basic_auth]

stub(Stops.Repo.Mock, :get, fn _ ->
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(
conn,
"authorization",
"Basic " <> Base.encode64("#{username}:#{password}")
),
params: %{
"plan" => %{
"from_latitude" => "#{Faker.Address.latitude()}",
"from_longitude" => "#{Faker.Address.longitude()}",
"to_latitude" => "#{Faker.Address.latitude()}",
"to_longitude" => "#{Faker.Address.longitude()}"
}
}
}
end

test "starts out with no 'View All Options' button", %{conn: conn, params: params} do
{: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
{:ok, view, _html} = live(conn, ~p"/preview/trip-planner?#{params}")

render_async(view)
view |> element("button[phx-value-index=\"0\"]", "Details") |> render_click()

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

test "clicking 'View All Options' button from details view closes it", %{
conn: conn,
params: params
} do
{:ok, view, _html} = live(conn, ~p"/preview/trip-planner?#{params}")

render_async(view)

view |> element("button[phx-value-index=\"0\"]", "Details") |> render_click()
view |> element("button", "View All Options") |> render_click()

refute render_async(view) =~ "View All Options"
end
end
end
8 changes: 4 additions & 4 deletions test/support/factories/trip_planner/trip_planner.ex
Original file line number Diff line number Diff line change
Expand Up @@ -141,22 +141,22 @@ defmodule Test.Support.Factories.TripPlanner.TripPlanner do

# OpenTripPlannerClient supports a greater number of route_type values than
# Dotcom does! Tweak that here.
defp limit_route_types(%OpenTripPlannerClient.Schema.Itinerary{legs: legs} = itinerary) do
def limit_route_types(%OpenTripPlannerClient.Schema.Itinerary{legs: legs} = itinerary) do
%OpenTripPlannerClient.Schema.Itinerary{
itinerary
| legs: Enum.map(legs, &limit_route_types/1)
}
end

defp limit_route_types(%OpenTripPlannerClient.Schema.Leg{route: route} = leg)
when route.type > 4 do
def limit_route_types(%OpenTripPlannerClient.Schema.Leg{route: route} = leg)
when route.type > 4 do
%OpenTripPlannerClient.Schema.Leg{
leg
| route: %OpenTripPlannerClient.Schema.Route{route | type: Faker.Util.pick([0, 1, 2, 3, 4])}
}
end

defp limit_route_types(leg), do: leg
def limit_route_types(leg), do: leg

def stop_named_position_factory do
%NamedPosition{
Expand Down
Loading