-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Detours channel #2908
base: main
Are you sure you want to change the base?
feat: Detours channel #2908
Conversation
Co-authored-by: Kayla Firestack <[email protected]>
a864baa
to
5af2080
Compare
5af2080
to
2c9023f
Compare
{:ok, %{conn: conn, socket: socket}} | ||
end | ||
|
||
def populate_db(conn) do |
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.
nit(non-blocking): btw I'm running into issues with this pattern in detours_controller_test
with populate_db_and_get_user
and would prefer that we try and do these(initializations) in the tests themselves, but I'll leave that up to you, as we can refactor later.
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.
I'd also prefer we don't hardcode Ids, but the whole (insert into db, pull out, update snapshot with id, put back into db) rigamarole is annoying, so no worries. Just a small concern.
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.
Mmmm I see I see. Let's talk when you get back. I can see the frustration with this, but want to confirm the better pattern (if there is one) before trying to edit
@@ -112,6 +131,10 @@ defmodule Skate.Detours.Detours do | |||
|
|||
def categorize_detour(_detour_context), do: :draft | |||
|
|||
@spec get_detour_route(detour :: map()) :: String.t() | |||
defp get_detour_route(%{state: %{"context" => %{"route" => %{"name" => route_name}}}}), |
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.
matching on the name could potentially cause some issues with matching routes where the id does not match the name ie SL4 vs 741.
Asana Ticket: https://app.asana.com/0/1205385723132845/1208600990461875
Depends on:
To try it out, I call
in
routeLadders.tsx
, anduseActiveDetours(socket)
,usePastDetours(socket)
, anduseDraftDetours(socket)
indetourListPage.tsx
.Needs: