feat: show new stop page layout & features - #3491
thecristen wants to merge 15 commits into
Conversation
joshlarson
left a comment
joshlarson
left a comment
There was a problem hiding this comment.
This looks great! And I'm excited to build more on top of it!
One of the comments is blocking-ish, but I decided that ish isn't enough to stop me approving... it won't break anything, and is easy enough to address as a follow-up in case you'd rather roll forward without addressing it here.
| @@ -0,0 +1,22 @@ | |||
| defmodule DotcomWeb.Live.StopUpcomingArrivals do | |||
There was a problem hiding this comment.
Question: Didn't we intentionally switch to a naming convention more akin to DotcomWeb.FooBarLive, as opposed to DotcomWeb.Live.FooBar?
I'm not overly attached to one or the other, but I do think it would be nice to be consistent.
There was a problem hiding this comment.
Updated (in a newly consolidated LiveView)!
| |> Map.put(:zoom, 17) | ||
| end | ||
|
|
||
| defp zoom_to_stop(config, _), do: config |
There was a problem hiding this comment.
Issue (blocking-ish): zoom_to_stop/2 can never be called with a second parameter that isn't a valid %Stop{} struct. While it is true that @stops_repo.get(stop_id) could return nil, the resulting stop is later called like so:
coordinates: [stop.longitude, stop.latitude]...which means that when stop is nil, then we'll crash before we get to the zoom_to_stop/2 call.
And Elixir 1.20 is smart enough to figure all that out and warn about it!
So I vote we get rid of this clause in order to get rid of an annoying warning.
There was a problem hiding this comment.
Done in feedback: remove unneeded clause
| <div class={["container", @new_stop_page? && "lg:flex xl:gap-xl md:gap-lg"]}> | ||
| <div :if={@new_stop_page?} class="lg:w-2/5 lg:order-last"> | ||
| <h2 class="mt-sm text-xl">{~t"Upcoming Departures"}</h2> | ||
| {live_render(@conn, DotcomWeb.Live.StopUpcomingArrivals, session: %{"stop_id" => @stop.id})} |
There was a problem hiding this comment.
Question: How come you decided to use separate LiveView's here, instead of putting the whole page into a single one? Do you envision combining them at some point, or keeping them separate?
There was a problem hiding this comment.
They're now together!
| |> Dotcom.StopAmenity.from_stop_facilities() | ||
|
|
||
| conn | ||
| |> assign(:new_stop_page?, Laboratory.enabled?(conn, :new_stop_page)) |
There was a problem hiding this comment.
Suggestion/Comment (non-blocking): We have a nifty plug that puts all of the flags into the assigns already, so we can just give this flag the atom of :new_stop_page? instead of :new_stop_page and skip this assign statement.
There was a problem hiding this comment.
Yes! Thanks, I'd totally forgotten about that.
This reverts commit 67bf452. # Conflicts: # lib/dotcom_web/templates/stop/show.html.heex
2 participants
Scope
Asana Ticket: TBA
Implementation
Using
live_render/3to embed LiveViews into a regular pageNote
This PR first fixes an issue where we couldn't use
live_render/3. It turned out using different plugs for regular pages vs live view pages was making the root layout kind of confused and the security signature not matching, or something.It's fixed by the first commit, which made the search bar break with a 403 error and necessitated the second commit:
All other POST requests seem to remain working as expected.
Kicking off the reworked Stop Page
Changed in both versions: Some styling tweaks shared between both, mostly the heading sizes
Added in new layout
StopMapLiveLiveView and added it to the new page where the map goes (thanks @joshlarson for the prior art in feat: Add a preview page with a stop map #2806 !)Screenshots
How to test
Try with and without the feature flag