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.

Sign up for GitHub

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

Jump to bottom

feat: Toggle between summary and details view in itinerary panel #2232

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: Toggle between summary and details view in itinerary panel #2232

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
feb0141
feat: Toggle between summary and details view in itinerary panel
joshlarson Nov 21, 2024
c62bc3c
cleanup: Remove extraneous `dbg`'s
joshlarson Nov 21, 2024
7227668
cleanup: Remove useless formatting change
joshlarson Nov 21, 2024
8505bc5
fix: Add :index attr to <.itinerary_group /> definition
joshlarson Nov 21, 2024
47fc2ed
cleanup: Refactor arguments to <.itinerary_panel_with_specific_result />
joshlarson Nov 21, 2024
340ac26
refactor: Pull itinerary results section out as a LiveComponent
joshlarson Nov 22, 2024
68e225b
refactor: Move itinerary_details_index into TripPlannerResultsSection
joshlarson Nov 22, 2024
bf1fae7
refactor: Make details_click_event on <.itinerary_group /> configurable
joshlarson Nov 22, 2024
38a1b26
cleanup: Remove unnecessary @group_id assign
joshlarson Nov 22, 2024
2608adf
Merge remote-tracking branch 'origin/main' into jdl/list-details-toggle
joshlarson Nov 22, 2024
925af40
wip: stub out some tests!
thecristen Nov 22, 2024
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
12 changes: 6 additions & 6 deletions lib/dotcom_web/components/trip_planner/itinerary_group.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ 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)

Expand Down Expand Up @@ -60,13 +58,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="show_itinerary_details"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In react-land, this would be a callback that would be explicitly passed into this component - onDetailsClick or something like that. I feel like we need some analogous strategy for that here, because I really don't like having "show_itinerary_details" being hard-coded in both this file and trip_planner.ex.

Do you have any suggestions? I can also experiment before merging, maybe by passing in a phx-details-click assign or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://hexdocs.pm/phoenix_live_view/Phoenix.LiveComponent.html#module-unifying-liveview-and-livecomponent-communication

☝️ this seems like a promising idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ha, I was going to link to this exact doc! can totally do that.

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 didn't end up passing in a callback because itinerary_group isn't a live component (pretty sure only live components can do anything meaningful with callbacks?). I did pass details_click_event and target in as assigns, which sort of accomplishes the same goal.

phx-value-group-id={@group_id}
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
67 changes: 65 additions & 2 deletions lib/dotcom_web/live/trip_planner.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ defmodule DotcomWeb.Live.TripPlanner do

use DotcomWeb, :live_view

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

Expand All @@ -28,6 +29,7 @@ defmodule DotcomWeb.Live.TripPlanner do
|> assign(:from, [])
|> assign(:to, [])
|> assign(:submitted_values, nil)
|> assign(:itinerary_details_index, nil)
|> assign_async(:results, fn ->
{:ok, %{results: nil}}
end)
Expand Down Expand Up @@ -73,7 +75,7 @@ defmodule DotcomWeb.Live.TripPlanner do
</div>
<.async_result :let={results} assign={@results}>
<div :if={results} class="w-full p-4">
<.itinerary_group :for={result <- results} {result} />
<.itinerary_panel results={results} details_index={@itinerary_details_index} />
</div>
</.async_result>
<.live_component
Expand All @@ -88,7 +90,65 @@ defmodule DotcomWeb.Live.TripPlanner do
"""
end

defp itinerary_panel(%{details_index: details_index} = assigns) do
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 think I want to pull itinerary_panel out into a separate live component, and have it handle the summary versus details toggling, rather than having that logic handled in the top-level TripPlanner live.

The one complicating factor is the map, since on mobile the map gets rendered in the middle of the details panel. That complicating factor is why I left it like this in this PR, but I sort of do want to figure out at least some strategy for this before merging.

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 wound up pulling out TripPlannerResultsSection instead, which includes the map. I think this is a passable solution for now, and might wind up being actually good? Interested to know what you think.

case details_index do
nil ->
~H"""
<.itinerary_panel_with_all_results results={@results} />
"""

_ ->
~H"""
<.itinerary_panel_with_specific_result results={@results} details_index={@details_index} />
"""
end
end

defp itinerary_panel_with_all_results(assigns) do
~H"""
<.itinerary_group :for={{result, index} <- Enum.with_index(@results)} index={index} {result} />
"""
end

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

~H"""
<div class="mt-30">
<button type="button" phx-click="show_itinerary_summary" 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("show_itinerary_details" = event, %{"index" => index_str} = params, socket) do
dbg(event)
joshlarson marked this conversation as resolved.
Show resolved Hide resolved
dbg(params)

{index, ""} = Integer.parse(index_str)

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

def handle_event("show_itinerary_summary", _params, socket) do
{:noreply, socket |> assign(:itinerary_details_index, nil)}
end

def handle_event(_event, _params, socket) do
{:noreply, socket}
end
Expand All @@ -112,7 +172,10 @@ defmodule DotcomWeb.Live.TripPlanner do
|> assign_async(:results, fn ->
case Dotcom.TripPlan.OpenTripPlanner.plan(data) do
{:ok, itineraries} ->
{:ok, %{results: ItineraryGroups.from_itineraries(itineraries)}}
{:ok,
%{
results: ItineraryGroups.from_itineraries(itineraries)
}}

error ->
error
Expand Down
Loading