Add the new metro map component to the preview trip planner

[anthonyshull]

Adding and removing markers works. When all markers are removed, the map recenters itself.

[joshlarson]

Why change lat/long from a named map to an array?

My concern is that while geojson does have the standard of [long, lat], [lat, long] is a lot more common in colloquial usage. (See Google Maps screenshot below, for instance). Because of that, just a list of numbers risks a very easy-to-make and hard-to-protect-against error of accidentally typing in [lat, long] instead of [long, lat].

Unless there's a good reason to follow the geojson standard, I'd strongly prefer to keep the named map all the way through.

[anthonyshull]

Maplibregl uses lng, lat. Since we're using that library, it makes sense to use that too. See the definition of center in the map config.

[joshlarson]

Ah yeah - my comment wasn't clear enough. I saw that MapLibre (which we don't control) and Metro (which we do) use geojson format. What I'd prefer is to move the geojson usage to be closer to the underlying library call, rather than leaking it all the way through the live component.

I made a suggested refactor that does address this, but if you don't like that refactor, then let's revisit this conversation.

[joshlarson]

I put a few comments in. I think all of them (at least discussing them) are approval-blockers for me.

[anthonyshull]

I replaced the integers with atoms if that makes it easier to read.

[joshlarson]

Suggested change

	pins={@pins}
	pins={[@from, @to]}

This, along with changing update_pin_in_socket to simply:

defp update_pin_in_socket(socket, [longitude, latitude], direction)
     when longitude != "" and latitude != "" do
  socket
  |> assign(direction, [String.to_float(longitude), String.to_float(latitude)])
end

defp update_pin_in_socket(socket, [longitude, latitude], direction)
     when longitude == "" or latitude == "" do
  socket
  |> assign(direction, [])
end

And adding

|> assign(:from, [])
|> assign(:to, [])

To the socket initialization in mount...

Would allow removing all of the place_pin logic, and most of the more complicated guts of the update_pin_in_socket functions as well.

(Net reduction of 41 lines of code 😇)

I think it would also be possible to combine update_pin_in_socket with update_*_pin, potentially for an even greater LoC reduction (and maybe clearer code), but this feels like a start.

[joshlarson]

Oooh actually, yes. If we replace all of the pin stuff with

 defp update_from_pin(socket, %{"from" => from}) do
   assign(socket, :from, to_geojson(from))
 end

 defp update_to_pin(socket, %{"to" => to}) do
   assign(socket, :to, to_geojson(to))
 end

 defp to_geojson(%{"longitude" => longitude, "latitude" => latitude})
      when longitude != "" and latitude != "" do
   [String.to_float(longitude), String.to_float(latitude)]
 end

 defp to_geojson(_coordinates) do
   []
 end

That gets us a net reduction of 53 lines of code (-54, but +1 because we have to set two assigns in the mount), and it resolves my concern with abruptly changing to geojson format in the middle of the live view, since this way we can keep the geojson format closer to the component where it's actually being used.

[anthonyshull]

had to add in another 6 lines of code because you have to have defaults for update_from_pin and update_to_pin for when the pattern doesn't match.

[joshlarson]

Couple of thoughts:

• I stand by not loving the pins logic. I put a suggestion for how to improve that logic into the code. Happy to pair on that if you'd like, and open to other ideas.
• Either as part of this PR or as a fast-follow, I think we need to add something about the CORS issue into the README, because otherwise local development will be broken.

Also, there's an issue where once the trip results appear, if :from and :to are close to lying on an east-west line, then the map won't change the zoom when it starts taking up just half of the screen, which means that the markers fall off of it.

IMO that's not blocking for this PR, but a bug to be aware of before releasing, so I wanted to flag it.

[anthonyshull]

📓 Lowering this because it was failing even though I am not editing any js/ts in this PR.