-
Notifications
You must be signed in to change notification settings - Fork 9
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: add activated-at
column to frontend detours list
#2911
base: main
Are you sure you want to change the base?
Conversation
12381a8
to
69c0258
Compare
activated_at
column to detours listactivated-at
column to frontend detours list
69c0258
to
c2eefbf
Compare
c2eefbf
to
2fa9a59
Compare
ca99c97
to
67d9ff5
Compare
2fa9a59
to
4e9b89c
Compare
@@ -75,12 +84,53 @@ export const DetourListPage = () => { | |||
visibility="All Skate users" | |||
classNames={["d-flex"]} | |||
/> | |||
<DetoursTable |
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.
Replacing DetoursTable
gets rid of the empty table state, which we need in case a miracle happens and there are absolutely no active detours
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.
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?
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.
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
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.
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>
+ )
+ })}
</>
)
}
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.
If you see a better way to have done this though, I'd be happy to pair or chat about a diff or suggestion!
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.
Hm I seeeeeeeee. So it seems primarily tricky because SimpleDetour is , and the typing could be made consistent if the activation details were optional attributes on SimpleDetour?
SimpleDetour {
id: DetourId
route: string
direction: string
name: string
intersection: string
updatedAt: number
activatedAt?: Date
}
This is what I was kinda thinking about when I suggested that activation info be nested within SimpleDetour as opposed to a wrapper around SimpleDetour. But I get that may have made things hairier on the backend. Hrmm. How do you feel about parsing the differently-typed data from the backend into a consistent "SimpleDetour" structure on the frontend?
4e9b89c
to
08685fb
Compare
} | ||
@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 |
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.
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
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'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.
assets/src/util/dateTime.ts
Outdated
const minute = second * 60 | ||
const hour = minute * 60 | ||
|
||
export const timeAgoLabelForDates = (start: Date, end: Date) => { |
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.
What's the purpose / difference between this and timeAgoLabel
? The input / output seems spiritually the same (even if they take different input types)
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.
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!
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.
how's this? 08685fb499b5cbfb8a076b3d605cd401047bfb5d..5693457f61f25ac61875cf5a6186178c957a6a09
#diff-f401aac6ed
(edit: oops, it got spread out over these commits,
- https://github.com/mbta/skate/compare/08685fb499b5cbfb8a076b3d605cd401047bfb5d..700b88734064884c7f03bdc1dbb6a3392794a6e6
- https://github.com/mbta/skate/compare/700b88734064884c7f03bdc1dbb6a3392794a6e6..220e1f94bc46d3a27d46b67d68f2fc329b07c4cc
- https://github.com/mbta/skate/compare/220e1f94bc46d3a27d46b67d68f2fc329b07c4cc..5693457f61f25ac61875cf5a6186178c957a6a09 )
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 am a little surprised by the snapshot change, so I'm gonna review that tomorrow (grr, snapshots >.>)
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.
Yeah, looks good! Just... grr snapshots
08685fb
to
700b887
Compare
700b887
to
220e1f9
Compare
220e1f9
to
5693457
Compare
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:
activated_at
column #2910