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: Add alert icons for stops on the map #111

Merged
merged 12 commits into from
Apr 4, 2024
Merged

feat: Add alert icons for stops on the map #111

merged 12 commits into from
Apr 4, 2024

Conversation

EmmaSimon
Copy link
Contributor

@EmmaSimon EmmaSimon commented Apr 2, 2024

Summary

Ticket: Stop icons on map reflect current alerts
Backend PR: mbta/mobile_app_backend#109

Simulator Screenshot - iPhone 15 Pro - 2024-04-02 at 13 08 14 Simulator Screenshot - iPhone 15 Pro - 2024-04-02 at 13 08 45

Show alert icons on stops for partial alerts, and red X icons for completely closed stops.

Testing

Added unit tests for kotlin alert to stop association, and for the map GeoJSON stop source generation

Base automatically changed from mth-nearby-stops-only to main April 2, 2024 19:02
@EmmaSimon EmmaSimon marked this pull request as ready for review April 2, 2024 21:37
@EmmaSimon EmmaSimon requested a review from a team as a code owner April 2, 2024 21:37
@EmmaSimon EmmaSimon requested a review from KaylaBrady April 2, 2024 21:37
@@ -44,13 +44,29 @@ class StopLayerGenerator {
static func getStopLayerIcon(_ locationType: LocationType) -> Value<ResolvedImage> {
switch locationType {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to move back to a single stop layer like I mentioned in this thread, but it doesn't allow you to use .zoom outside of a top level .step expression for some reason, so having it nested one layer deeper in a .match doesn't seem possible.

Copy link
Collaborator

@KaylaBrady KaylaBrady left a comment

Choose a reason for hiding this comment

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

Ran locally, looks great! A few questions & suggestions to consider. Are there any station closures to see? May be worth creating one in the alerts dev UI if not!

Copy link
Collaborator

@KaylaBrady KaylaBrady left a comment

Choose a reason for hiding this comment

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

Thanks for making those updates!

return alerts.filter { Alert.serviceDisruptionEffects.contains(it.effect) }
}

private fun getServiceStatus(
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (non-blocking): this has a lot of important logic, would it be worth unit testing independently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, possibly. The behavior in the existing tests was meant to cover everything here, so it would probably be a bit redundant, but it's not a bad idea.

@EmmaSimon
Copy link
Contributor Author

RE: Station alerts, there aren't any real ones rn, but I made one in dev alerts UI:

Simulator Screenshot - iPhone 15 Pro - 2024-04-04 at 13 58 54

@EmmaSimon EmmaSimon merged commit 7a6ed82 into main Apr 4, 2024
5 checks passed
@EmmaSimon EmmaSimon deleted the es-stop-alerts branch April 4, 2024 20:28
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.

3 participants