Migrate everything, trim out (most) dead code#1
Conversation
General theory here was to keep some of the light rail specific data but also break out interfaces that can be used when this is eventually pulled into Orbit. I imagine that more work will be needed then but I think this puts us in a good place overall.
jkap
force-pushed
the
jkap/migration-part-1
branch
from
b78a78d to
8881c5b
Compare
andrewdolce
left a comment
andrewdolce
left a comment
There was a problem hiding this comment.
Overall this looks good to me. I appreciate the amount of things that were trimmed out in this first pass.
Most of my questions have to do with light rail / Glides-specific things that are still in here. I know that was intentional on your part, just wondering if we could get away with removing some of them? In particular dropping TripEnd from TrainLoc might be an easy win (unless I'm overlooking something.)
Ultimately though I'm OK if this stuff lands here for now and gets moved out later. Maybe we can chat about it a bit when you're next available.
| * Configuration point: Green-line-specific merge-point logic. | ||
| * Checks whether this train aligns with this segment at Kenmore, Copley, and Lechmere. | ||
| * For rail operations, pass a custom `alignsWithSegment` function to the Ladder component. |
There was a problem hiding this comment.
question: Passing in alignsWithSegment from the outside makes sense to me, just curious about the thinking around leaving in this Green-line-specific default? Is this something you envision moving out of the shared repo eventually, or is there some reason I'm overlooking that would prevent us from defining this on the outside too?
| export interface PredictionsSelection { | ||
| stationId: StationId; | ||
| directionId: DirectionId; | ||
| } | ||
|
|
||
| export interface SideBarSelection { | ||
| routeId: RouteId; | ||
| consist: Consist; | ||
| } |
There was a problem hiding this comment.
suggestion/thought: Feel free to push back on this if you think it would be too much work, wasted effort, or doesn't make sense, but I'm wondering if we can rename these things to be a bit more generic? Perhaps something like:
PredictionSelection --> StationSelection
SideBarSelection --> VehicleSelection
My thoughts are that since the side bar and prediction tray are not yet part of the shared ladder, from the ladder's perspective all the user is really doing is selecting a station+direction or selecting a specific vehicle, and what happens next could be left up to the application?
That said, it's possible that Glides and Orbit will be very aligned on what these selections are used for, and that we'll end up making a shared sidebar and prediction view, so leaving them as-is wouldn't be the end of the world? And I suppose we could rename them later if the use cases differ.
| export const allStationsArray: Station[] = [ | ||
| // GLX Medford | ||
| buildStation("place-mdftf", "Medford/Tufts", 42.408179, -71.117185, "Medford"), | ||
| buildStation("place-balsq", "Ball Square", 42.400241, -71.111278, "Ball Sq"), |
There was a problem hiding this comment.
question: This is again something very Green-line specific (as is the list of car numbers and the routes/segments). Do you see us pulling these things out eventually and having the application provide them as configuration?
| // DirectionId matches GTFS | ||
| export enum DirectionId { | ||
| Westbound = 0, | ||
| Eastbound = 1, | ||
| } |
There was a problem hiding this comment.
thought: I don't have a great suggestion here, but I think an unfortunate consequence of combining heavy rail and light rail is that directional terminology differs by line, e.g. pretty sure "northbound" and "southbound" are more common on the RL. But making this more generic might be a pain in the neck or add a lot of complexity. I'll think on it...
| latLng: LatLng | null; | ||
| heading: number | null; | ||
| timestamp: DateTime | null; | ||
| trip: TripEnd | null; |
There was a problem hiding this comment.
question: Does the ladder UI use this for anything other than determining if a vehicle is on a revenue trip? Trying to look through and see... If not then I'm wondering if we could remove any references to TripEnd since that's a Glides-specific thing, and just give TrainLoc its own revenue property?
Maybe not a change for this PR if the plan is to strip additional Glides stuff out when we start fitting this with Orbit, but thought I'd ask.
2 participants
Asana Task: 🪇 Break out Glides ladder
General theory here was to keep some of the light rail specific data but also break out interfaces that can be used when this is eventually pulled into Orbit. I imagine that more work will be needed then but I think this puts us in a good place overall.
Will be adding GHA workflows to run tests and etc in another PR.
I'm sorry that this is going to suck to review. Here's a summary:
largely unchanged
avoidOverlaps.tstrainHeight.tsladderLabel.tsxminor changes
ladder.tsxalignsWithSegmentprop, allowing non-light rail to provide its own function for determining which segment to render a train on (will be most useful for RL)new
types.tspredictions.tsxandsideBar.tsxsince these areladderPage.tsxspecific files that we didn't want to move into a shared component, but we still need to pass data from them intoLadderas props.additional notes
Checklist
( )Light & dark mode( )Desktop & mobile sizes( )Chromium( )Firefox( )Safari( )Commits free of internal data( )PR description free of internal data( )Logging free of internal data( )Has tests( )Doesn't need tests( )Tests deferred (with justification)( )Okayed the plan for the feature (e.g. the design files, or the Asana task)( )Reviewed the feature as implemented (e.g. on dev-green, or saw screenshots)( )No review needed