Skip to content
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(HomeMapView): Draw alerting segments as dashed lines #123

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

KaylaBrady
Copy link
Collaborator

@KaylaBrady KaylaBrady commented Apr 9, 2024

Summary

Ticket: Style alerting segments on the map

What is this PR for?
Draw alerting segments on map as dashed lines.

Testing

What testing have you done?

Updated / added unit tests and ran locally.
Still something funky going on that is preventing a GL alert from showing up as expected, but OL & RL are working. Hoping for this incremental step to be reviewed first and will dive into the GL alert as a follow-up.

image

@KaylaBrady KaylaBrady force-pushed the kb-draw-alerting-segments branch from 24a8262 to 61ac3f0 Compare April 9, 2024 20:52
@KaylaBrady KaylaBrady marked this pull request as ready for review April 9, 2024 20:55
@KaylaBrady KaylaBrady requested a review from a team as a code owner April 9, 2024 20:55
@KaylaBrady KaylaBrady requested review from EmmaSimon and removed request for a team April 9, 2024 20:55
@KaylaBrady
Copy link
Collaborator Author

Update: GL alerting segment showing! Restarting my locally running backend got it to appear, so I suspect there is something going on in the backend that is preventing new alerts from being pushed - I'll make a ticket to investigate that more.

There is also still an issue here since I would expect Gov't center to haymarket to show as alerting - will look into that further.
image

Copy link
Contributor

@EmmaSimon EmmaSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Nice work. Having overlapping routes only display starting at the station they split at looks so much better. Now commuter rail needs a similar treatment, but it looks like it'll be pretty straightforward to add cross-route handling, could that be at all related to the green line follow up issues?

Edit: Oops, didn't refresh before posting that, sounds unrelated.

static func createRouteLayer(route: Route) -> LineLayer {
var routeLayer = LineLayer(
id: Self.getRouteLayerId(route.id),
static func createRouteLayer(route: Route) -> [LineLayer] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel weird about leaving the function name singular when it's returning multiple, but there's already a plural. And something like createRouteLayerPair or createRouteLayerSet are still subject to change and too close to type names... Maybe instead createRouteLayers could be createAllRouteLayers and this createRouteLayers?

Base automatically changed from kb-alerting-segments to main April 10, 2024 20:38
@KaylaBrady KaylaBrady force-pushed the kb-draw-alerting-segments branch from e75c60c to f0c9335 Compare April 10, 2024 20:42
@KaylaBrady KaylaBrady merged commit 2b54ead into main Apr 11, 2024
4 of 5 checks passed
@KaylaBrady KaylaBrady deleted the kb-draw-alerting-segments branch April 11, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants