feat(StopPageDepartures): Omit uncommon Green Line branches/headsigns with no upcoming service #1786

thecristen · 2023-11-02T15:59:39Z

Summary of changes

Asana Ticket: Omit uncommon Green Line branches/headsigns with no upcoming service

There are a couple extra commits because our code wasn't using the V3 API's canonical attribute for route patterns. Most of this PR's file changes are due to this.

The main part of this PR, in the final commit, checks the canonical value for subway route patterns against the list of departures to determine whether to hide a specific headsign. Additionally, if all the headsigns for a given route card end up hidden, the route card itself will also be hidden.

Before and After:

In the early morning when the other Green Line branches run trains this way, those schedules/predictions should show up on this page, though I haven't captured a screenshot for this yet.

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.

skyqrose

I have a bunch of comments, but they're all "I would have done this differently" minor refactoring suggestions. It all makes sense and looks good.

skyqrose · 2023-11-13T13:58:04Z

apps/site/assets/ts/models/route-patterns.ts

+} from "../stop/stop-redesign-loader";
+import { DepartureInfo } from "./departureInfo";
+
+type RoutePatternGroup = GroupedRoutePatterns[keyof GroupedRoutePatterns];


Maybe this file should define

export interface RoutePatternWithPolyline extends Omit<RoutePattern, "representative_trip_polyline"> { representative_trip_polyline: Polyline; } export type RoutePatternId = string; // I'm assuming the groups are by id but maybe it's by headsign? export type RoutePatternGroup = Record< RoutePatternId, { direction_id: DirectionId; route_patterns: RoutePatternWithPolyline[]; }, >; export type GroupedRoutePatterns = Record<string, RoutePatternGroup>;

and stop-redesign-loader should reference this file. The types were defined there before cuz that's where they were used, but having this general model file reference that specific use case seems backwards.

Also, there's two layers of grouping RoutePatternGroup and GroupedRoutePatterns, and it's unclear what the difference is. Type aliases for the string keys could help, or putting some hint in the type name could help.

I changed the group keys to be references to the relevant objects (Route["id"] and RoutePattern["headsign"] in this case) and the end result should be way cleaner, thanks a million 🥳

skyqrose · 2023-11-13T14:01:33Z

apps/site/assets/ts/models/route-patterns.ts

+export type RoutePatternGroupEntries = [
+  keyof RoutePatternGroup,
+  RoutePatternGroup[keyof RoutePatternGroup]
+][];


If this is a pattern you expect to use again, you could consider defining in some util file somewhere (maybe helpers/object.ts)

export type ValueOf<T extends Object> = T[keyof T]; export type Entries<T extends Object> = [keyof T, ValueOf<T>];

skyqrose · 2023-11-13T14:19:57Z

apps/site/assets/ts/models/route-patterns.ts

+    ].map(([, { route_patterns: routePatterns }]) =>
+      Math.min(...routePatterns.map(rp => rp.sort_order))
+    );
+    return orderA - orderB;


Would lodash sortBy() work for this?

skyqrose · 2023-11-13T14:21:55Z

apps/site/assets/ts/models/route-patterns.ts

+  return isNonCanonical && departures.length === 0;
+};
+
+// eslint-disable-next-line import/prefer-default-export


Why? Sometimes the reason for disabling a lint rule is obvious, but this one isn't, and could use a comment.

skyqrose · 2023-11-13T14:37:51Z

apps/site/assets/ts/stop/components/DepartureCard.tsx

@@ -19,24 +19,10 @@ const DepartureCard = ({
 }: {
  alertsForRoute: Alert[];
  departuresForRoute: DepartureInfo[];
-  routePatternsByHeadsign: GroupedRoutePatterns[keyof GroupedRoutePatterns];
+  routePatternsByHeadsign: RoutePatternGroupEntries;


This PR moves the filtering+sorting out of this component and into the caller. That means that it needs to take the entries type now instead of the record type, which I think is a bit more awkward. I tend to prefer passing around the more data-like record type as long as possible, and only transform it to the display-like sorted list as late as possible to be closer to where it's displayed (though this is a very loose rule and there could be good reasons I don't see to do the processing earlier).

skyqrose · 2023-11-13T14:53:53Z

apps/site/assets/ts/models/route-patterns.ts

+
+const isNoncanonicalAndNoDepartures = (
+  routePatterns: RoutePatternWithPolyline[],
+  departures: DepartureInfo[]


This function doesn't work purely on route patterns, it also uses departure info, so maybe it shouldn't belong in the general models/route-patterns file. This calculation is pretty specific to what you want to show on the stops page, so maybe it should go with the stops page components instead.

skyqrose · 2023-11-13T18:03:01Z

apps/site/assets/ts/models/route-patterns.ts

+      minBy(routePatterns, "sort_order")?.sort_order
+  );
+
+export default { sortedGroupedRoutePatterns };


Is this needed when the definition above already has export const sortedGroupedRoutePatterns?

thecristen requested a review from a team as a code owner November 2, 2023 15:59

thecristen requested review from i0sea and removed request for a team November 2, 2023 15:59

thecristen assigned i0sea Nov 2, 2023

thecristen added the dev-blue Deploy to dev-blue label Nov 2, 2023

thecristen temporarily deployed to dev-blue November 2, 2023 18:32 — with GitHub Actions Inactive

thecristen assigned kotva006 Nov 8, 2023

thecristen force-pushed the cbj/remove-uncommon-headsigns-without-service branch from 79516bb to 3eb9895 Compare November 9, 2023 14:22

thecristen temporarily deployed to dev-blue November 9, 2023 14:22 — with GitHub Actions Inactive

thecristen assigned skyqrose Nov 9, 2023

thecristen requested a review from skyqrose November 9, 2023 21:34

skyqrose approved these changes Nov 13, 2023

View reviewed changes

thecristen temporarily deployed to dev-blue November 13, 2023 17:09 — with GitHub Actions Inactive

thecristen temporarily deployed to dev-blue November 13, 2023 17:29 — with GitHub Actions Inactive

thecristen force-pushed the cbj/remove-uncommon-headsigns-without-service branch from 8730c68 to e539877 Compare November 13, 2023 17:42

thecristen temporarily deployed to dev-blue November 13, 2023 17:42 — with GitHub Actions Inactive

skyqrose approved these changes Nov 13, 2023

View reviewed changes

Base automatically changed from cbj/better-unserved-stops to master November 14, 2023 19:42

thecristen added 9 commits November 14, 2023 14:44

feat(RoutePattern): add canonical attribute

24853c1

feat(frontend): add canonical prop to RoutePattern

7d20c61

refactor: extract route pattern sorting function

62bf002

refactor(StopPageDepartures): sort route patterns

191191e

feat(StopPageDepartures): omit noncanon headsign

e04c3c8

more lodashery for easier sorting

57b6e56

move isNoncanonAndNoDepartures to the one place it's actually needed

59eb3d2

move relevant types to route-patterns, add clarity for group keys

7637863

move logic back down to departure card, more moving

61fa99e

thecristen force-pushed the cbj/remove-uncommon-headsigns-without-service branch from e539877 to 61fa99e Compare November 14, 2023 19:44

thecristen temporarily deployed to dev-blue November 14, 2023 19:44 — with GitHub Actions Inactive

thecristen removed the dev-blue Deploy to dev-blue label Nov 14, 2023

fix broken unit tests

cbe4075

thecristen merged commit dd76a63 into master Nov 14, 2023
17 of 18 checks passed

thecristen deleted the cbj/remove-uncommon-headsigns-without-service branch November 14, 2023 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(StopPageDepartures): Omit uncommon Green Line branches/headsigns with no upcoming service #1786

feat(StopPageDepartures): Omit uncommon Green Line branches/headsigns with no upcoming service #1786

thecristen commented Nov 2, 2023 •

edited

Loading

skyqrose left a comment

skyqrose Nov 13, 2023

thecristen Nov 13, 2023

skyqrose Nov 13, 2023

skyqrose Nov 13, 2023

skyqrose Nov 13, 2023

skyqrose Nov 13, 2023

skyqrose Nov 13, 2023

skyqrose Nov 13, 2023

feat(StopPageDepartures): Omit uncommon Green Line branches/headsigns with no upcoming service #1786

feat(StopPageDepartures): Omit uncommon Green Line branches/headsigns with no upcoming service #1786

Conversation

thecristen commented Nov 2, 2023 • edited Loading

Summary of changes

General checks

skyqrose 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

thecristen commented Nov 2, 2023 •

edited

Loading