Itinerary detail map #2258
Itinerary detail map #2258
Conversation
anthonyshull
commented
Dec 10, 2024
anthonyshull
commented
lib/dotcom/trip_plan/map.ex
Outdated
| defp tailwind_color("#003DA5"), do: "blue-500" | ||
| defp tailwind_color("#80276C"), do: "purple-500" | ||
| defp tailwind_color("#00843D"), do: "green-500" | ||
| defp tailwind_color("#ED8B00"), do: "orange-500" | ||
| defp tailwind_color("#DA291C"), do: "red-500" | ||
| defp tailwind_color("#7C878E"), do: "silver-500" |
There was a problem hiding this comment.
📓 For now, we have to use the Metro colors because it will only accept those. We can change Metro to pass in an arbitrary hex code.
| <.async_result :let={results} assign={@results}> | ||
| <%= if results && @itinerary_selection != :summary do %> | ||
| <% {:detail, %{itinerary_group_index: itinerary_group_index, itinerary_index: itinerary_index}} = @itinerary_selection %> | ||
| <% itinerary = Enum.at(results, itinerary_group_index).itineraries |> Enum.at(itinerary_index) %> | ||
| <% itinerary_map = TripPlan.Map.itinerary_map(itinerary) %> | ||
| <% lines = TripPlan.Map.get_lines(itinerary_map) %> | ||
| <% points = TripPlan.Map.get_points(itinerary_map) %> | ||
| <.live_component | ||
| module={MbtaMetro.Live.Map} | ||
| id="trip-planner-map" | ||
| class={[ | ||
| "h-64 md:h-96 w-full", | ||
| "relative overflow-none row-span-2", | ||
| @itinerary_selection == :summary && "hidden md:block" | ||
| ]} | ||
| config={@map_config} | ||
| lines={lines} | ||
| pins={[@from, @to]} | ||
| points={points} | ||
| /> | ||
| <% else %> | ||
| <.live_component | ||
| module={MbtaMetro.Live.Map} | ||
| id="trip-planner-map" | ||
| class={[ | ||
| "h-64 md:h-96 w-full", | ||
| "relative overflow-none row-span-2", | ||
| @itinerary_selection == :summary && "hidden md:block" | ||
| ]} | ||
| config={@map_config} | ||
| pins={[@from, @to]} | ||
| /> | ||
| <% end %> |
There was a problem hiding this comment.
📓 None of this will be here after we change the state management. It must be embedded like this because the results component includes the map.
| |> Enum.reject(&is_nil/1) | ||
| end | ||
|
|
||
| defp invert_coordinate([a, b]), do: [b, a] |
There was a problem hiding this comment.
Nit (non-blocking): This kind of thing is exactly why I would prefer to see us standardize on %{latitude: lat, longitude: long} (or %{lat: lat, long: long} or whatever), rather than having to keep track of which intermediate objects use [long, lat] versus which ones use [lat, long].
My ideal would be that any intermediate, dotcom-controlled data is always stored as a struct, and is only translated to [lat, long] or [long, lat] at the boundary, like right before being passed into MapLibreGL, or right after being parsed out of GTFS, etc.
There was a problem hiding this comment.
This is right before it is being passed into maplibre gl.
|
|
||
| defp invert_coordinate([a, b]), do: [b, a] | ||
|
|
||
| defp invert_coordinate(_), do: nil |
There was a problem hiding this comment.
If we ever get a coordinate that isn't a tuple it will crash the process otherwise.
| @@ -150,4 +197,13 @@ defmodule Dotcom.TripPlan.Map do | |||
| def z_index(%{current: idx, start: idx}), do: 100 | |||
| def z_index(%{current: idx, end: idx}), do: 100 | |||
| def z_index(%{}), do: 0 | |||
|
|
|||
| defp tailwind_color("#003DA5"), do: "blue-500" | |||
There was a problem hiding this comment.
I'm torn about this. Most of the other places where we translate things from GTFS to color, we do it based on the route, but here we're pulling the color out of the polyline directly.
If it turns out that we can get this to work, should we use the colors directly in otther contexts too?
There was a problem hiding this comment.
The color that's in the polyline came from the associated route, it's just parsed roundaboutly because legacy....
|
This PR introduces a bug, where the map disappears while OTP is loading. For me, at least right now, that bug is approval-blocking. I think I have a solution to that bug, which also cleans up some of the messier code here (I know it's short-term, but still better to avoid even short-term mess when possible). I'm posting a more detailed comment in-line. |
lib/dotcom_web/components/trip_planner/trip_planner_results_section.ex
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks pretty great!
I made a few comments, but the big one that's holding up approval for me is the map-flickering bug, which I think would be reasonably-elegantly solved by this change, but as long as it's fixed, I'm not too worried about how.
