-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stop ID autocomplete #1057
base: master
Are you sure you want to change the base?
Stop ID autocomplete #1057
Conversation
93dbbc6
to
e66f4b9
Compare
53dcbf0
to
9f8b809
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly comments / suggestions, looks good overall.
@spec stops_or_gtfs_stops_by_search_string(String.t()) :: [Stop.t() | GtfsStop.t()] | ||
def stops_or_gtfs_stops_by_search_string(string) do | ||
# See https://github.blog/engineering/like-injection/ | ||
sanitized_string = Regex.replace(~r/([\%_])/, string, fn _, x -> "\\#{x}" end) <> "%" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion / question
Maybe a silly question, but could we just remove all %
's? And maybe even _
's too. I wouldn't expect either of those characters to ever show up in a stop name, ID, or description.
Also, it looks like you anchor the search to the start of the string since %
is only appended and not prepended to the search. I think that's ok but just wanted to check that it was an intentional choice.
matching_gtfs_stops = | ||
gtfs_stops_query |> Repo.all() |> Enum.filter(&(!MapSet.member?(arrow_stop_ids, &1.id))) | ||
|
||
Enum.concat(matching_stops, matching_gtfs_stops) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion
Would it make sense to sort the results in some way? For example, alphabetically or by Jaro distance from the user-entered string.
(Though since the search is over 3 different fields in 2 different tables, this could get complicated as you'd need to do something along the lines of
matches
|> Enum.sort_by(fn stop ->
[stop.name, stop.id, stop.desc]
|> Enum.map(&String.jaro_distance(&1, string)
|> Enum.max()
end)
so feel free to leave it as-is if that seems like too much)
Phoenix.HTML.Form.input_value(assigns.field.form, :stop) || | ||
Phoenix.HTML.Form.input_value(assigns.field.form, :gtfs_stop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion / question
Does this mean this component is safe to use only in a form that also has stop
and gtfs_stop
inputs?
My understanding of LiveView is still pretty tenuous, but this feels like a sort of hidden dependency for this component.
Is there a way to pass these values directly and explicitly into StopInput
, as additional attrs, instead of having it reach outside of itself under the assumption that these other sibling inputs exist and have the specified names?
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
||
@spec option_for_stop(Stop.t() | GtfsStop.t()) :: {String.t(), String.t()} | ||
defp option_for_stop(%Stop{stop_id: stop_id, stop_desc: stop_desc, stop_name: stop_name}), | ||
do: {"#{stop_id} - #{stop_desc || stop_name}", stop_id} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue
The Arrow shuttle stops table's stop_desc
column is non-null, and empty strings are truthy in Elixir, so stop_desc || stop_name
will never evaluate to stop_name
.
I'm not sure if we allow stop_desc
to be an empty string, but if we do then this would need to be something like
do: {"#{stop_id} - #{stop_desc || stop_name}", stop_id} | |
do: {"#{stop_id} - #{if(stop_desc != "", do: stop_desc, else: stop_name)}", stop_id} |
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