feat: Added route ladders to new minimal ladders page #2583
Conversation
Coverage of commit
|
| @@ -52,7 +52,9 @@ export const AppRoutes = () => { | |||
| }, [path, setPath]) | |||
|
|
|||
| const vehiclesByRouteIdNeeded = | |||
| openView === OpenView.Late || location.pathname === "/" | |||
| openView === OpenView.Late || | |||
There was a problem hiding this comment.
This seems kind of brittle (it was before too, but now that there are three conditions, it seems especially so), but I don't have any great ideas for how to de-brittle-ify it. I wish there was some way that the view itself could tell app "I need vehiclesByRouteId".
Possibly having useVehicles be called within the page components themselves? I had a thought that that wouldn't be possible because of the socket, but the socket should be accessible within the page components by way of useContext.
This is a non-blocking comment, and probably shouldn't be addressed in this PR anyway, since that would run the risk of creating PR bloat.
| useTimepoints(selectedRouteIds) | ||
|
|
||
| const { socket } = useContext(SocketContext) | ||
| const alerts = useAlerts(socket, selectedRouteIds) |
There was a problem hiding this comment.
Refactoring the alerts code here worries me a bit because it's completely untested, on main as well as here.
I tried clicking around to test this - I wasn't able to find any alerts visible in skate (either local or prod), so I didn't get a good manual check, but then just for fun, I went ahead and deleted it entirely, and once I removed all the unused imports and things, all of the tests still passed. I tried out the same thing on main, and same deal.
I would feel more comfortable with this if we snuck another PR in front of this one that added some tests that validated the alerts logic.
There was a problem hiding this comment.
Merged main, and the useAlerts test still passes ✅
There was a problem hiding this comment.
This comment is my big blocker for approval on this PR - I'm happy to pair on it if that would be helpful!
I have loosely-held opinions about the questions you asked in the PR description:
I lowered some logic from the LadderPage into the RouteLadders component to avoid stubbing out identical logic in MinimalLadderPage. Does that seem rational?
Yeah - When I was thinking that I would pick this story up, that's exactly how I envisioned doing it. Well... sort of - I hadn't looked closely at the RouteLadders and LadderPage code, yet, so I had a slightly different vision based on a slightly wrong guess about how they worked, but IMO, I think this is the right way to do it.
Also, is this PR too big?
Eh.... that's subjective. For my sensibilities, it's hovering right on the cusp. If if were me doing it, I would probably have broken it up into two PR's (plus the third pre-PR that I asked for in my blocking comment, which I do think should be separate from this one):
- (My requested new one) Add test coverage for alerts, and anything else that might be missing
- Refactor to lower some of the logic from
LadderPageintoRouteLadders - Add MinimalLadderPage
But it's not tooo tooooo big to combine the "Refactor to lower" work with the "Add MinimalLadderPage" work, so I'm not going to like... insist that you do extra work to split it into two PR's
assets/src/components/ladderPage.tsx
Outdated
| routeId: RouteId | ||
| ): Route | undefined => (routes || []).find((route) => route.id === routeId) | ||
|
|
||
| // This is tested but never used? |
There was a problem hiding this comment.
Ayayaye, I deleted the tests and didn't delete the function def, which is especially foolish! I meant to leave all references to the function intact and raise the question in the PR. Because it is unused, I'm leaning towards deletion and will do so unless instructed otherwise
Coverage of commit
|
Coverage of commit
|
Asana Ticket: https://app.asana.com/0/1205385723132845/1207162343766763
I lowered some logic from the LadderPage into the RouteLadders component to avoid stubbing out identical logic in MinimalLadderPage. Does that seem rational?
Also, is this PR too big?