feat: Show "Depart at" buttons and show the correct itinerary on click #2238

joshlarson · 2024-11-26T20:08:19Z

Adds these buttons, which toggle between different versions of the same itinerary.

Summary of changes

Asana Ticket: Trip Planner Preview | "Depart at" buttons

General checks

Are the changes organized into self-contained commits with descriptive and well-formatted commit messages? This is a good practice that can facilitate easier reviews.
Testing. Do the changes include relevant passing updates to tests? This includes updating screenshots. Preferably tests are run locally to verify that there are no test failures created by these changes, before opening a PR.
Tech debt. Have you checked for tech debt you can address in the area you're working in? This can be a good time to address small issues, or create Asana tickets for larger issues.

New UI, or substantial UI changes

Cross-browser compatibility is less of an issue now that we're no longer supporting IE, but changes still need to work as expected in Safari, Chrome, and Firefox.
Are interactive elements accessible? This includes at minimum having relevant keyboard interactions and visible focus, but can also include verification with screen reader testing.
Other accessibility checks such as sufficient color constrast, or whether the layout holds up at 200% zoom level.

New endpoints, or non-trivial changes to current endpoints

Have we load-tested any new pages or internal API endpoints that will receive significant traffic? See load testing docs
If this change involves routes, does it work correctly with pertinent "unusual" routes such as the combined Green Line, Silver Line, Foxboro commuter rail, and single-direction bus routes like the 170?

And change its input from `itinerary` to `itineraries` so that it can be responsible for choosing which itinerary to display

lib/dotcom_web/components/live_components/itinerary_detail.ex

anthonyshull

I think this would probably be better as a function component since it has so little state. I'm just nervous about having state in multiple places...sometimes in the live view, sometimes in live components. With things like the map which can potentially have a lot of state, needs a lot of interactivity, and is very self-contained, it makes more sense to make it a live component. It would probably help to have some kind of rule-of-thumb about the number of attributes and callbacks that make a live component make sense. I don't think this component meets a minimal criteria.

anthonyshull · 2024-12-02T14:17:11Z

lib/dotcom_web/components/live_components/itinerary_detail.ex

+      type="button"
+      class={[
+        "border border-brand-primary rounded px-2.5 py-1.5 mr-2 text-brand-primary text-lg",
+        "hover:bg-brand-primary-lightest #{@background_class}"


You can just do:

"hover:bg-brand-primary-lightest", @background_class

But, you might even want to do:

"hover:bg-brand-primary-lightest bg-transparent", @active && "bg-brand-primary-lightest"

That allows you to remove two lines of code.

anthonyshull · 2024-12-02T14:26:57Z

test/dotcom_web/live/trip_planner_test.exs

+        update_trip_details(base_itinerary, trip_id: "trip_id_1", start_time: ~T[09:26:00]),
+        update_trip_details(base_itinerary, trip_id: "trip_id_2", start_time: ~T[10:46:00])


We try to avoid magic numbers, names, times, etc. in tests. Reading this test, I wonder what is special about these two times. If we're just testing that the text in the button reflects the time we pass it, we can assign a random time.

If we're testing that the later time is the one that appears, we can create a random time for the first then add time to that, etc.

I went the way I did here because I prefer not to have logic in tests, since that makes it harder to see exactly what the test is testing. In this case, the logic would be the conversion from ~T[09:26:00] into the string "9:26AM".

I do agree that having ~T[09:26:00] and "9:26AM" be closer to each other in the test file would make them feel less magic. Lemme try a quick refactor, and then let's talk if we still don't love that.

anthonyshull · 2024-12-02T14:33:05Z

test/dotcom_web/live/trip_planner_test.exs

+  def stub_otp_results(itineraries) do
+    expect(OpenTripPlannerClient.Mock, :plan, fn _ ->
+      {:ok, %OpenTripPlannerClient.Plan{itineraries: itineraries}}
+    end)
+  end


These should probably be private functions since they won't be used anywhere outside of this file.

anthonyshull · 2024-12-02T14:36:22Z

test/dotcom_web/live/trip_planner_test.exs

+    # Uhhh the OTP factory will generate with any route_type value but our
+    # parsing will break with unexpected route types
+    itineraries =
+      OtpFactory.build_list(3, :itinerary)
+      |> Enum.map(&limit_route_types/1)
+
+    stub_otp_results(itineraries)


Don't we already do this in a factory?

dotcom/test/support/factories/trip_planner/trip_planner.ex

Line 12 in 8266c56

|> limit_route_types()

If not, we should probably fix the factory rather than add that code here (since it applies to all code that uses the OtpFactory and not just this test).

I don't think we do this in a factory (I didn't add this - I just moved it), and I do think we should. I was thinking that fixing that could be a follow-up PR, so that the depart-at buttons can still go-live.

That's fine as long as we do it later. I know the factory does this, but it also parses it. Might be worth trying to use the factory directly:

dotcom/test/support/factories/trip_planner/trip_planner.ex

Line 10 in 8266c56

def itinerary_factory do

anthonyshull · 2024-12-02T14:41:48Z

test/dotcom_web/live/trip_planner_test.exs

-      expect(OpenTripPlannerClient.Mock, :plan, fn _ ->
-        {:ok, %OpenTripPlannerClient.Plan{itineraries: []}}
-      end)
+      stub_otp_results([])


Since you're only using this in one place, I would just write this as an expectation in this test. That would allow you to remove one of the two functions above.

stub_otp_results is called in two places - once here and once in stub_populated_otp_results. I guess I could inline it in both places, but I wanted to avoid duplicating 👇

expect(OpenTripPlannerClient.Mock, :plan, fn _ -> {:ok, %OpenTripPlannerClient.Plan{itineraries: itineraries}} end)

joshlarson · 2024-12-02T15:51:18Z

I think this would probably be better as a function component since it has so little state. I'm just nervous about having state in multiple places...sometimes in the live view, sometimes in live components.

I'd love to talk about this more, since I actually found this easier to reason about by having the state located closer to where that state is used. I got confused by having the state managed by the live view, but then having the interaction still be facilitated by the buttons within the details component.

I could sort of see having the state still live at the live view level, and get managed via callback, but I think that the itinerary_details component would still need to be a live component in order to use callbacks.

anthonyshull · 2024-12-02T17:32:43Z

I think this would probably be better as a function component since it has so little state. I'm just nervous about having state in multiple places...sometimes in the live view, sometimes in live components.

I'd love to talk about this more, since I actually found this easier to reason about by having the state located closer to where that state is used. I got confused by having the state managed by the live view, but then having the interaction still be facilitated by the buttons within the details component.

I could sort of see having the state still live at the live view level, and get managed via callback, but I think that the itinerary_details component would still need to be a live component in order to use callbacks.

If you don't capture clicks with target = myself then they'll go to the parent. That way the logic for the entire live view is in a single place instead of in multiple places. It gives us a nice pattern of expectation that attributes flow down and events bubble up. From the docs:

Generally speaking, you should prefer functional components over live components, as they are a simpler abstraction, with a smaller surface area. The use case for live components only arises when there is a need for encapsulating both event handling and additional state.

anthonyshull · 2024-12-02T17:38:04Z

In metro, the only reason the map and datepicker are live components is because they have to interact with external libraries and manage state in JS. Even the map, when clicked, actually sends the event to the parent live view. So, if you were going to add a pin where the user clicked the live view would add that pin in a callback.

https://github.com/mbta/mbta_metro/blob/ad3b265264dd57f4e4284ae0a010d6869b732047/lib/mbta_metro/live/map.ex#L14

joshlarson · 2024-12-02T18:55:46Z

Talked offline with the following conclusion:

We'll make ItineraryDetail into a function component as part of this PR.
Fast-follow PR to make TripPlannerResultsSection into a function component as well, with state managed exclusively by the TripPlanner live view.

joshlarson · 2024-12-02T19:10:36Z

lib/dotcom_web/components/live_components/trip_planner_results_section.ex

+    {:ok,
+     socket
+     |> assign(:expanded_itinerary_index, nil)
+     |> assign(:selected_itinerary_detail_index, 0)}


Per this comment, both of these bits of state are going to get moved up to the TripPlanner level in a follow-up PR.

…e-showing an itinerary

…imit_route_types each time

thecristen

Thanks for addressing the feedback!

joshlarson added 4 commits November 26, 2024 15:07

prefactor: Convert <.itinerary_detail /> into a live component

72dd393

And change its input from `itinerary` to `itineraries` so that it can be responsible for choosing which itinerary to display

feat: Show "Depart at" buttons and show the correct itinerary on click

63fa14e

style: Style depart-at buttons correctly (entirely with Tailwind)

3d0f7dd

refactor: Extract <.depart_at_button /> as a sub-component

a8b79da

thecristen reviewed Nov 26, 2024

View reviewed changes

lib/dotcom_web/components/live_components/itinerary_detail.ex Outdated Show resolved Hide resolved

joshlarson added 4 commits November 26, 2024 16:57

style: Format the "Depart at" text correctly

a0a6823

cleanup: Remove some useless assigns

3d194d4

tests: Add tests that validate itinerary-toggling behavior

f144a64

refactor/cleanup: Rename selected_trip_index to selected_itinerary_index

7103053

joshlarson marked this pull request as ready for review November 27, 2024 21:33

joshlarson requested a review from a team as a code owner November 27, 2024 21:33

joshlarson requested a review from thecristen November 27, 2024 21:34

joshlarson enabled auto-merge (squash) November 27, 2024 21:41

joshlarson disabled auto-merge November 27, 2024 21:41

joshlarson enabled auto-merge (squash) November 27, 2024 21:41

fix: Remove TODO tag, which was causing Credo to fail

9dedde5

anthonyshull reviewed Dec 2, 2024

View reviewed changes

refactor: Convert itinerary_details to a function component

98ae545

joshlarson added 3 commits December 2, 2024 14:00

tests: Co-locate exlicit testing values

fbd6177

Merge remote-tracking branch 'origin/main' into jdl/depart-at-buttons

eb6bfc2

cleanup: Make test helper functions private

52f867f

joshlarson commented Dec 2, 2024

View reviewed changes

fix: Don't preserve selected_itinerary_detail_index when hiding and r…

b2e48a7

…e-showing an itinerary

joshlarson requested a review from anthonyshull December 2, 2024 21:41

joshlarson added 2 commits December 2, 2024 17:08

tests: Use existing factory for otp_itineraries rather than calling l…

11537cc

…imit_route_types each time

cleanup: Use faker for trip times and trip ID's in tests

669039f

thecristen approved these changes Dec 3, 2024

View reviewed changes

thecristen disabled auto-merge December 3, 2024 17:09

thecristen merged commit 5ab1881 into main Dec 3, 2024
16 of 17 checks passed

thecristen deleted the jdl/depart-at-buttons branch December 3, 2024 17:10

joshlarson mentioned this pull request Dec 9, 2024

refactor: Convert TripPlannerResultsSection to a function component #2252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Show "Depart at" buttons and show the correct itinerary on click #2238

feat: Show "Depart at" buttons and show the correct itinerary on click #2238

joshlarson commented Nov 26, 2024 •

edited

Loading

anthonyshull left a comment

anthonyshull Dec 2, 2024

anthonyshull Dec 2, 2024

anthonyshull Dec 2, 2024

anthonyshull Dec 2, 2024

joshlarson Dec 2, 2024

anthonyshull Dec 2, 2024

anthonyshull Dec 2, 2024

joshlarson Dec 2, 2024

anthonyshull Dec 2, 2024

anthonyshull Dec 2, 2024

joshlarson Dec 2, 2024

joshlarson commented Dec 2, 2024

anthonyshull commented Dec 2, 2024

anthonyshull commented Dec 2, 2024

joshlarson commented Dec 2, 2024

joshlarson Dec 2, 2024 •

edited

Loading

thecristen left a comment

		update_trip_details(base_itinerary, trip_id: "trip_id_1", start_time: ~T[09:26:00]),
		update_trip_details(base_itinerary, trip_id: "trip_id_2", start_time: ~T[10:46:00])

feat: Show "Depart at" buttons and show the correct itinerary on click #2238

feat: Show "Depart at" buttons and show the correct itinerary on click #2238

Conversation

joshlarson commented Nov 26, 2024 • edited Loading

Summary of changes

General checks

New UI, or substantial UI changes

New endpoints, or non-trivial changes to current endpoints

anthonyshull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlarson commented Dec 2, 2024

anthonyshull commented Dec 2, 2024

anthonyshull commented Dec 2, 2024

joshlarson commented Dec 2, 2024

joshlarson Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

thecristen left a comment

Choose a reason for hiding this comment

joshlarson commented Nov 26, 2024 •

edited

Loading

joshlarson Dec 2, 2024 •

edited

Loading