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

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

Conversation

joshlarson
Copy link
Contributor

Screen.Recording.2024-11-21.at.11.52.59.AM.mov

Summary of changes

Asana Ticket: Trip Planner Preview | Toggle between list and details views

@joshlarson joshlarson requested a review from a team as a code owner November 21, 2024 16:54
@@ -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.

<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.

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.

@thecristen thecristen added the dev-green Deploy to dev-green label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-green Deploy to dev-green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants