feat: add interactive stop to map for create shuttle stop and edit shuttle stop #999
Conversation
Coverage of commit
|
Coverage of commit
|
meagharty
force-pushed
the
meag/stop-map-view
branch
from
bff73b5
to
6b4a1b4
Compare
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
meagharty
requested review from
a team and
jzimbel-mbta
and removed request for
a team
| @@ -7,7 +7,7 @@ | |||
| </:actions> | |||
| </.header> | |||
|
|
|||
| <.table id="stops" rows={@stops} row_click={&JS.navigate(~p"/stops/#{&1}")}> | |||
There was a problem hiding this comment.
This row_click attribute wasn't working previously
| @@ -24,9 +24,6 @@ | |||
| <:col :let={stop} label="At street"><%= stop.at_street %></:col> | |||
| <:col :let={stop} label="Last updated"><%= format_timestamp(stop.updated_at) %></:col> | |||
| <:action :let={stop}> | |||
| <div class="sr-only"> | |||
| <.link navigate={~p"/stops/#{stop}"}>Show</.link> | |||
There was a problem hiding this comment.
This page didn't exist before anyway. The screenreader-only tag hid the visible broken link.
| @@ -16,6 +16,8 @@ | |||
| "leaflet": "^1.9.4", | |||
| "phoenix": "file:../deps/phoenix", | |||
| "phoenix_html": "file:../deps/phoenix_html", | |||
| "phoenix_live_react": "file:../deps/phoenix_live_react", | |||
| "phoenix_live_view": "file:../deps/phoenix_live_view", | |||
There was a problem hiding this comment.
I referenced https://github.com/phoenixframework/phoenix_live_view/blob/v0.20.1/guides/introduction/installation.md for fixing up the existing live_view setup
| @@ -0,0 +1,22 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
This root/app/live setup is from https://github.com/phoenixframework/phoenix_live_view/blob/v0.20.1/guides/introduction/installation.md#layouts
There was a problem hiding this comment.
Nit: Could you nest this in a stop_live directory so its path mirrors that of the source file in lib that it's testing?
(Or move stop_live.ex up a level)
| <meta charset="utf-8"/> | ||
| <meta http-equiv="X-UA-Compatible" content="IE=edge"/> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"/> | ||
| <meta name="csrf-token" content={Plug.CSRFProtection.get_csrf_token()} /> |
There was a problem hiding this comment.
Should app.js be referenced in a <script> tag somewhere after this?
(Looks like it's added via content_tag(:script ...) in each of the layout templates, but maybe it would make more sense to do that in this root-level file if it's always used?)
There was a problem hiding this comment.
I'm going to keep this script tag as-is inside the body tag (as it was before this PR). I don't want to make further changes to the setup with React and LiveView in this PR.
assets/src/app.tsx
Outdated
| .getAttribute("content") | ||
| const liveSocket = new LiveSocket("/live", Socket, { | ||
| hooks, | ||
| // longPollFallbackMs: 2500, |
There was a problem hiding this comment.
Should this be removed? Or is it good to keep around?
(I couldn't find documentation on this option in a quick skim of the phoenix_live_view docs / the live_socket.js source.)
There was a problem hiding this comment.
This option was configured in the generated app.js in a new phoenix project with live_view (template source code in phoenix here).
I uncommented this with an update for local development.
This issue describes trade-offs of falling back to long polling if the websocket fails to connect (including local development): phoenixframework/phoenix#5741, a helpful comment in particular: phoenixframework/phoenix#5741 (comment)
config/config.exs
Outdated
| env: %{ | ||
| "NODE_PATH" => | ||
| Enum.join( | ||
| [Path.expand("../deps", __DIR__), Path.expand("../assets/node_modules", __DIR__)], |
There was a problem hiding this comment.
Any reason why /assets/node_modules needed to be added to NODE_PATH?
There was a problem hiding this comment.
I can't recall why so I can only assume this was from debugging the liveview setup. I removed this addition and I didn't see any differences testing the application locally.
There was a problem hiding this comment.
Looks like a good deal of markup is shared between this template and app.html.heex, would it make sense / would it be possible to consolidate it in another template?
| type Stop = { | ||
| stop_name: string | ||
| stop_desc: string | ||
| stop_lat: number | ||
| stop_lon: number | ||
| } |
There was a problem hiding this comment.
Any reason for type Stop = {...} instead of interface Stop {...}? They're equivalent if I remember correctly, but I'm used to a soft standard of people using interface.
There was a problem hiding this comment.
I'll change this. To your point the Typescript community when setting a standard prefers interface for better error messages.
This is a personal preference for type that I have trouble kicking from a time when I was writing both Typescript and types in F#. I like that types can't be re-declared, can be used for objects, functions, and other types (primitives, unions, tuples), and are extended with & (over extends which feels OOPish).
Coverage of commit
|
Summary of changes
Asana Ticket: Implement "Create Shuttle Stop" and "Edit Shuttle Stop" map view - display draft stop on map
phx-changetriggers a socket.assigns update, which re-renders the React map with the new propsphx-submit) but it was an awkward experience especially on createReviewer Checklist