feat(shuttles): Make the direction description a drop-down menu instead of free text #1095
Conversation
rudiejd
requested review from
a team and
jzimbel-mbta
and removed request for
a team
rudiejd
force-pushed
the
feat/make-direction-desc-dropdown
branch
from
e4f0ffa to
8a6d697
Compare
There was a problem hiding this comment.
@jzimbel-mbta I jumped in here because I was curious about inputs_for/f_route change propagation.
| type="select" | ||
| label="Direction Description" | ||
| prompt="Choose a value" | ||
| phx-change="direction_desc_changed" |
There was a problem hiding this comment.
| phx-change="direction_desc_changed" |
You should be able to remove this and the related handle_event calls functions. You don't need to have this input handle it's own change -- I double-checked that this worked locally (just in case there was an issue with the @form level handling of the f_route). I don't see another reason or edge case as to why it would need to be handled here.
| @@ -7,7 +7,7 @@ defmodule Arrow.Shuttles.Route do | |||
| field :suffix, :string | |||
| field :destination, :string | |||
| field :direction_id, Ecto.Enum, values: [:"0", :"1"] | |||
| field :direction_desc, :string | |||
| field :direction_desc, Ecto.Enum, values: [:Inbound, :Outbound, :North, :South, :East, :West] | |||
There was a problem hiding this comment.
Do we need to add a migration or some other mitigation to drop any existing routes? I think this will be a breaking change in dev/prod when trying to render test data.
There was a problem hiding this comment.
These are actually the only values in use in prod/dev. And switching this to an Ecto.Enum should still have it serialized as a varchar. I can make a migration just to be extra safe though.
There was a problem hiding this comment.
"update the values in prod/dev" was a valid mitigation to me so if that's already the case then great 👍
| @@ -451,6 +475,31 @@ defmodule ArrowWeb.ShuttleViewLive do | |||
| end | |||
| end | |||
|
|
|||
| defp update_second_direction(socket, first_direction) do | |||
There was a problem hiding this comment.
Ah okay I see why there's custom change handling here.
Not a designer but this side effect feels surprising to me -- generally I'm not a fan of side effects in UIs. It doesn't reverse itself either (as in, if I change the second, the first doesn't change).
I could see us validating somewhere (like the Shuttle.changeset fn) that the direction pairs line up though. And/or adding to def mount(%() ...) (~line 299) to set default Inbound/Outbound.
There was a problem hiding this comment.
yeah, definitely not a designer either 😅
My idea was that I still wanted to allow people to change the second direction if they don't match up, because I think technically it is valid to have Inbound / North for example. I can get rid of this if that is more annoying than helpful though
There was a problem hiding this comment.
I'm not sure it's worth the extra complexity and possible user confusion on the edit page especially, like if the user doesn't notice/realize the other input changed.
We could ask Benji in the arrow slack channel
Summary of changes
Asana Ticket: [extra] 🏹 Change direction_desc to drop-down (in Shuttle Definitions)
Problem:
Direction description on the shuttle form was a free text field, but in reality there are a fixed number of potential descriptions for a direction.
Solution:
Make direction description a drop-down menu instead of a free-text box.
bonus: I made the second direction automatically populate with a sane default - for example if you choose "Inbound" it automatically populates with "Outbound"
Reviewer Checklist