Stop ID autocomplete #1057
Stop ID autocomplete #1057
Conversation
lemald
force-pushed
the
lem-stop-autocomplete
branch
from
93dbbc6 to
e66f4b9
Compare
lemald
force-pushed
the
lem-stop-autocomplete
branch
from
53dcbf0 to
9f8b809
Compare
lemald
requested review from
a team,
dks-mbta and
jzimbel-mbta
and removed request for
a team and
dks-mbta
| # This should only change if the selected value actually changes, | ||
| # not just when a user is typing and options change. | ||
| assigns = | ||
| assign( | ||
| assigns, | ||
| :options, | ||
| if is_nil(assigns.stop) || !Ecto.assoc_loaded?(assigns.stop) do | ||
| [] | ||
| else | ||
| [option_for_stop(assigns.stop)] | ||
| end | ||
| ) |
There was a problem hiding this comment.
Question
Bear with me as I continue to muddle through the live component lifecycle & data-flow models...
So, is the idea here that this expression in render/1 sets the initial options for the live_select, and the logic in handle_event("live_select_change", ...) updates the options by sending them directly to the live_select and having it essentially override the original list it received from the initial render?
This is a little weird to me, as the options attr passed by this component to live_select can be replaced without this component's knowledge... It feels very not-declarative 🤨
Based on the diagram here, it seems like render/1 will be called again if this wrapper component, or any of its ancestors, see a change in state. (Which makes sense to me as someone coming from React-land)
Would that cause the options list to be reset to the result of this expression, if it was previously updated by a "live_select_change" event?
Do we want that behavior?
There was a problem hiding this comment.
I think the key thing is that the handle_event only updates the state of the LiveSelect component, not the StopInput component. When the user selects a value, that triggers an actual change of the value on the form field, which causes the re-render of the StopInput component. That said, I agree it's kind of weird and counterintuitive, I'll have to give this some more thought.
There was a problem hiding this comment.
Yeah I guess this is ok in Phoenix, if it provides a send_update function that seems to be intended for this use case.
It still feels weird to me though—seems analogous to mutating a prop within a component in React, which is an anti-pattern there. Still adjusting to this new framework!
Basically, I would have expected handle_event to simply update the StopInput component's internal state (which I think is stored in the socket?), which would cause a re-render with the new state, causing the new data to "flow down" to its LiveSelect child. Instead of directly telling the LiveSelect to re-render itself with a new options value.
There was a problem hiding this comment.
Yeah, I'm also adjusting to LiveView after spending a lot of time in React-land. Let me poke around and see if it's somehow possible to do this by updating state on socket as you suggested.
There was a problem hiding this comment.
So I could use assign to update in the socket but it's basically the same as send_update, which just updates the assigns under the hood. Basically my options are to make this an assign of the inner LiveSelect component or of my component. If I were to do the latter, than the parent would have to choose the initial options, which I don't want it to have to do. I feel like in React this is the sort of thing I would reach to state for, basically for things that don't make sense as props but that components need to track internally, but I don't really see a LiveView equivalent to state. So, tl;dr I'm going to leave it as is for now, but this is thought-provoking and I'll have to consider if this is actually the best way of doing things going forward.
lemald
force-pushed
the
lem-stop-autocomplete
branch
from
c7236e9 to
ea8bdbf
Compare
lemald
force-pushed
the
lem-stop-autocomplete
branch
from
cb87266 to
101a38f
Compare
Co-authored-by: Jon Zimbel <[email protected]>
There was a problem hiding this comment.
I think you addressed my pending comment about the empty string handling while I was finishing up! So feel free to ignore/resolve that one.
My only other comment was a reply on this thread, but I think this is good to go as-is. I'm not yet confident enough in my understanding of LiveView lifecycle to be sure I'm giving the right advice on that aspect.
Summary of changes
Asana Ticket: 🏹 Implement "Create/Edit Shuttle Route Definition" - Enhanced Stop Search
I'm still hoping to get some unit testing of the component behavior itself working, but running into issues - see discussion on Slack. However, everything else should be in place for reviewing.
Reviewer Checklist