feat: add `activated-at` column to frontend detours list #2911

firestack · 2024-12-21T04:47:52Z

This uses the work in #2910 to fix the Activated At column to use the real activated at time, in the Activated Detours table.

Asana Ticket: https://app.asana.com/0/0/1208989312907930/f
Depends on:

feat(ex/skate/detours): add activated_at column #2910

hannahpurcell · 2025-01-09T13:52:10Z

assets/src/components/detourListPage.tsx

@@ -75,12 +84,53 @@ export const DetourListPage = () => {
            visibility="All Skate users"
            classNames={["d-flex"]}
          />
-          <DetoursTable


Replacing DetoursTable gets rid of the empty table state, which we need in case a miracle happens and there are absolutely no active detours

It doesn't seem like it would be too complex to use the status === DetourStatus.Active to determine whether the extra column should be added, which should be all we need for this feature. Is there something else that was changed with the replacement?

Replacing DetoursTable gets rid of the empty table state, which we need in case a miracle happens and there are absolutely no active detours

Good catch, Thanks!

I am slightly surprised we don't have a test to have caught this

It doesn't seem like it would be too complex to use the status === DetourStatus.Active to determine whether the extra column should be added, which should be all we need for this feature. Is there something else that was changed with the replacement?

I think doing this logic within the component would make it unreasonably messy, and would require a larger refactor to clean up the component to accept different data types, in my opinion, this way was just generally simpler and straight forward (and in my opinion points in the direction that we should not have made the table into it's own component when we did).

I tried mocking up what it would be like to do this within the existing table component to decipher which types are being used at which points, which I think introduces unnecessary conditional logic to access the types.

Commit ID: 1738660c49f3a596fd94026a3f0a3bd56f8379ed Change ID: xpltoxzkmrnpwsqvmylwtvlskrotnluw Author : Kayla Firestack <[email protected]> (2025-01-09 09:36:38) Committer: Kayla Firestack <[email protected]> (2025-01-09 10:01:41) (no description set) diff --git a/assets/src/components/detourListPage.tsx b/assets/src/components/detourListPage.tsx index 4a8a847d39..2e21a7a35e 100644 --- a/assets/src/components/detourListPage.tsx +++ b/assets/src/components/detourListPage.tsx @@ -84,57 +84,12 @@ visibility="All Skate users" classNames={["d-flex"]} /> - <Table - hover={!!detours.active} - className={"mb-5 c-detours-table"} - variant={"active-detour"} - > - <thead className="u-hide-for-mobile"> - <tr> - <th className="px-3 py-4">Route and direction</th> - <th className="px-3 py-4 u-hide-for-mobile"> - Starting Intersection - </th> - <th className="px-3 py-4 u-hide-for-mobile"> - {timestampLabelFromStatus(DetourStatus.Active)} - </th> - <th className="px-3 py-4 u-hide-for-mobile">Est. Duration</th> - </tr> - </thead> - <tbody> - {detours.active - ? detours.active.map((detour, index) => ( - <tr - key={index} - onClick={() => onOpenDetour(detour.details.id)} - > - <td className="align-middle p-3"> - <div className="d-flex"> - <RoutePill routeName={detour.details.route} /> - <div className="c-detours-table__route-info-text d-inline-block"> - <div className="pb-1 fs-4 fw-semibold"> - {detour.details.name} - </div> - <div className="c-detours-table__route-info-direction fs-6"> - {detour.details.direction} - </div> - </div> - </div> - </td> - <td className="align-middle p-3 u-hide-for-mobile"> - {detour.details.intersection} - </td> - <td className="align-middle p-3 u-hide-for-mobile"> - {timeAgoLabelForDates(detour.activatedAt, epochNow)} - </td> - <td className="align-middle p-3 u-hide-for-mobile"> - {detour.estimatedDuration} - </td> - </tr> - )) - : null} - </tbody> - </Table> + <DetoursTable + data={detours.active} + status={DetourStatus.Active} + onOpenDetour={onOpenDetour} + classNames={["mb-5"]} + /> {userInTestGroup(TestGroups.DetoursPilot) && ( <> <Title diff --git a/assets/src/components/detoursTable.tsx b/assets/src/components/detoursTable.tsx index f8fe3d9824..77af66700c 100644 --- a/assets/src/components/detoursTable.tsx +++ b/assets/src/components/detoursTable.tsx @@ -1,19 +1,30 @@ import React from "react" import { Table } from "react-bootstrap" import { RoutePill } from "./routePill" -import { useCurrentTimeSeconds } from "../hooks/useCurrentTime" -import { timeAgoLabel } from "../util/dateTime" -import { SimpleDetour } from "../models/detoursList" +import useCurrentTime, { useCurrentTimeSeconds } from "../hooks/useCurrentTime" +import { timeAgoLabel, timeAgoLabelForDates } from "../util/dateTime" +import { ActivatedDetour, SimpleDetour } from "../models/detoursList" import { EmptyDetourTableIcon } from "../helpers/skateIcons" import { joinClasses } from "../helpers/dom" +import { ActivateDetour } from "./detours/activateDetourModal" -interface DetoursTableProps { - data: SimpleDetour[] | undefined +interface DetoursTableBaseProps { onOpenDetour: (detourId: number) => void - status: DetourStatus classNames?: string[] } +type DetoursTableProps = DetoursTableBaseProps & + ( + | { + data: ActivatedDetour[] | undefined + status: DetourStatus.Active + } + | { + data: SimpleDetour[] | undefined + status: DetourStatus.Closed | DetourStatus.Draft + } + ) + export enum DetourStatus { Draft = "draft", Active = "active", @@ -51,11 +62,26 @@ <th className="px-3 py-4 u-hide-for-mobile"> {timestampLabelFromStatus(status)} </th> + {status === DetourStatus.Active && ( + <th className="px-3 py-4 u-hide-for-mobile">Est. Duration</th> + )} </tr> </thead> <tbody> {data ? ( - <PopulatedDetourRows data={data} onOpenDetour={onOpenDetour} /> + status === DetourStatus.Active ? ( + <PopulatedDetourRows + status={status} + data={data} + onOpenDetour={onOpenDetour} + /> + ) : ( + <PopulatedDetourRows + status={status} + data={data} + onOpenDetour={onOpenDetour} + /> + ) ) : ( <EmptyDetourRows message={`No ${status} detours.`} /> )} @@ -65,36 +91,67 @@ const PopulatedDetourRows = ({ data, + status, onOpenDetour, }: { - data: SimpleDetour[] onOpenDetour: (detourId: number) => void -}) => { - const epochNowInSeconds = useCurrentTimeSeconds() +} & ( + | { + data: ActivatedDetour[] + status: DetourStatus.Active + } + | { + data: SimpleDetour[] + status: DetourStatus.Closed | DetourStatus.Draft + } +)) => { + const epochNow = useCurrentTime() + const epochNowInSeconds = epochNow.valueOf() / 1000 return ( <> - {data.map((detour, index) => ( - <tr key={index} onClick={() => onOpenDetour(detour.id)}> - <td className="align-middle p-3"> - <div className="d-flex"> - <RoutePill routeName={detour.route} /> - <div className="c-detours-table__route-info-text d-inline-block"> - <div className="pb-1 fs-4 fw-semibold">{detour.name}</div> - <div className="c-detours-table__route-info-direction fs-6"> - {detour.direction} + {data.map((detour, index) => { + let simpleDetour: SimpleDetour + if (status == DetourStatus.Active) { + simpleDetour = (detour as ActivatedDetour).details + } else { + simpleDetour = detour as SimpleDetour + } + + return ( + <tr key={index} onClick={() => onOpenDetour(simpleDetour.id)}> + <td className="align-middle p-3"> + <div className="d-flex"> + <RoutePill routeName={simpleDetour.route} /> + <div className="c-detours-table__route-info-text d-inline-block"> + <div className="pb-1 fs-4 fw-semibold"> + {simpleDetour.name} + </div> + <div className="c-detours-table__route-info-direction fs-6"> + {simpleDetour.direction} + </div> </div> </div> - </div> - </td> - <td className="align-middle p-3 u-hide-for-mobile"> - {detour.intersection} - </td> - <td className="align-middle p-3 u-hide-for-mobile"> - {timeAgoLabel(epochNowInSeconds, detour.updatedAt)} - </td> - </tr> - ))} + </td> + <td className="align-middle p-3 u-hide-for-mobile"> + {simpleDetour.intersection} + </td> + <td className="align-middle p-3 u-hide-for-mobile"> + {status === DetourStatus.Active + ? timeAgoLabelForDates( + (detour as ActivatedDetour).activatedAt, + epochNow + ) + : timeAgoLabel(epochNowInSeconds, simpleDetour.updatedAt)} + </td> + {status === DetourStatus.Active && ( + <td className="align-middle p-3 u-hide-for-mobile"> + {(detour as ActivatedDetour).estimatedDuration} + </td> + )} + </tr> + ) + })} </> ) }

If you see a better way to have done this though, I'd be happy to pair or chat about a diff or suggestion!

hannahpurcell · 2025-01-09T14:45:32Z

lib/skate/detours/detours.ex

-    }
+  @spec db_detour_to_detour(status :: detour_type(), Detour.t()) :: DetailedDetour.t() | nil
+  def db_detour_to_detour(%{} = db_detour) do
+    db_detour_to_detour(categorize_detour(db_detour), db_detour)
  end

  def db_detour_to_detour(invalid_detour) do


This has kinda been replaced by db_detour_to_detour/2 on lines 101-7, because the only reason this function would execute is if the detour is simply not a map at all (probably if it's nil?) I'm not sure this function would still be useful in that case, but if it is, then the error message should be changed

I'm still thinking about this, but I'm going to re-request review while I consider this, to unblock the other conversations in case I can iterate on those faster.

hannahpurcell · 2025-01-09T14:51:31Z

lib/skate/detours/detour.ex

+    def from(
+          status,
+          %{
+            state: %{
+              "context" => %{
+                "route" => %{"name" => route_name, "directionNames" => direction_names},
+                "routePattern" => %{"headsign" => headsign, "directionId" => direction_id},
+                "nearestIntersection" => nearest_intersection
+              }
+            }
+          } = db_detour
+        ) do


I like moving this logic here! I'm trying to think of an alternative name but don't have a great one... Maybe an explanatory comment would be good though? I see from and assumed it was something like a kernel operation I didn't know about

I personally would like to keep this convention (and do it more 😅), I did pick it up from Rust [which got inspiration from some other language IIRC] (there's a From/Into convention, where you can do

1.into<String>() // or String::from(1)

and I think this is a good way to do conversions.

It could be named from_detour_snapshot though. I think from is still the best descriptor/word though.

hannahpurcell · 2025-01-09T14:55:43Z

assets/src/util/dateTime.ts

+const minute = second * 60
+const hour = minute * 60
+
+export const timeAgoLabelForDates = (start: Date, end: Date) => {


What's the purpose / difference between this and timeAgoLabel? The input / output seems spiritually the same (even if they take different input types)

The main difference is the addition of "Just now" but I made a separate function for "development speed" sake, I'll see about condensing these!

how's this? https://github.com/mbta/skate/compare/08685fb499b5cbfb8a076b3d605cd401047bfb5d..700b88734064884c7f03bdc1dbb6a3392794a6e6

(edit: oops, a little more too it that got missed, https://github.com/mbta/skate/compare/700b88734064884c7f03bdc1dbb6a3392794a6e6..220e1f94bc46d3a27d46b67d68f2fc329b07c4cc https://github.com/mbta/skate/compare/220e1f94bc46d3a27d46b67d68f2fc329b07c4cc..5693457f61f25ac61875cf5a6186178c957a6a09 )

I am a little surprised by the snapshot change, so I'm gonna review that tomorrow (grr, snapshots >.>)

firestack force-pushed the kf/asn/actiavated-at-frontend branch from 12381a8 to 69c0258 Compare December 21, 2024 04:52

firestack changed the title ~~feat(ex/skate/detours): add activated_at column to detours list~~ feat: add activated-at column to frontend detours list Dec 21, 2024

firestack force-pushed the kf/asn/actiavated-at-frontend branch from 69c0258 to c2eefbf Compare December 21, 2024 04:58

firestack force-pushed the kf/asn/actiavated-at-frontend branch from c2eefbf to 2fa9a59 Compare January 6, 2025 21:15

firestack mentioned this pull request Jan 6, 2025

feat: add estimated-duration column to frontend detours list #2912

Open

firestack force-pushed the kf/asn/activated-at-column branch from ca99c97 to 67d9ff5 Compare January 6, 2025 21:40

firestack force-pushed the kf/asn/actiavated-at-frontend branch from 2fa9a59 to 4e9b89c Compare January 6, 2025 21:40

firestack marked this pull request as ready for review January 6, 2025 21:45

firestack requested a review from a team as a code owner January 6, 2025 21:45

Base automatically changed from kf/asn/activated-at-column to main January 8, 2025 15:59

firestack assigned hannahpurcell Jan 8, 2025

hannahpurcell reviewed Jan 9, 2025

View reviewed changes

firestack force-pushed the kf/asn/actiavated-at-frontend branch from 4e9b89c to 08685fb Compare January 9, 2025 14:14

hannahpurcell reviewed Jan 9, 2025

View reviewed changes

hannahpurcell assigned firestack and unassigned hannahpurcell Jan 9, 2025

firestack force-pushed the kf/asn/actiavated-at-frontend branch from 08685fb to 700b887 Compare January 9, 2025 20:56

firestack assigned hannahpurcell Jan 9, 2025

firestack requested a review from hannahpurcell January 9, 2025 21:08

firestack force-pushed the kf/asn/actiavated-at-frontend branch from 700b887 to 220e1f9 Compare January 9, 2025 21:10

feat: add activated-at column to frontend detours list

5693457

firestack force-pushed the kf/asn/actiavated-at-frontend branch from 220e1f9 to 5693457 Compare January 9, 2025 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `activated-at` column to frontend detours list #2911

feat: add `activated-at` column to frontend detours list #2911

firestack commented Dec 21, 2024 •

edited

Loading

hannahpurcell Jan 9, 2025

hannahpurcell Jan 9, 2025

firestack Jan 9, 2025 •

edited

Loading

firestack Jan 9, 2025

firestack Jan 9, 2025

hannahpurcell Jan 9, 2025

firestack Jan 9, 2025

hannahpurcell Jan 9, 2025

firestack Jan 9, 2025

firestack Jan 9, 2025

hannahpurcell Jan 9, 2025

firestack Jan 9, 2025

firestack Jan 9, 2025 •

edited

Loading

firestack Jan 9, 2025

feat: add activated-at column to frontend detours list #2911

Are you sure you want to change the base?

feat: add activated-at column to frontend detours list #2911

Conversation

firestack commented Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firestack Jan 9, 2025 • edited Loading

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

firestack Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `activated-at` column to frontend detours list #2911

feat: add `activated-at` column to frontend detours list #2911

firestack commented Dec 21, 2024 •

edited

Loading

firestack Jan 9, 2025 •

edited

Loading

firestack Jan 9, 2025 •

edited

Loading