8000 fix: trip display on disrupted branches by BrandonTR · Pull Request #883 · mbta/mobile_app · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: trip display on disrupted branches #883

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

Merged
merged 2 commits into from
Apr 17, 2025
Merged

Conversation

BrandonTR
Copy link
Contributor

Summary

Ticket: Group by direction | Add in alerts data

This attempts to solve for this case in figma.

iOS
- [ ] If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?
- [ ] Add temporary machine translations, marked "Needs Review"

android
- [ ] All user-facing strings added to strings resource in alphabetical order
- [ ] Expensive calculations are run in withContext(Dispatchers.Default) where possible (ideally in shared code)

Testing

Filled in a unit test that tests this scenario.

@BrandonTR BrandonTR requested a review from a team as a code owner April 10, 2025 21:58
@BrandonTR BrandonTR requested a review from KaylaBrady April 10, 2025 21:58
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.

Approving as a step forward since this does improve the behavior for branching stops by showing non-disrupted branches! We can continue to iterate on this based on the follow-up items from the miro board discussion as separate tickets.

trip.headsign,
UpcomingFormat.Some(
format,
UpcomingFormat.SecondaryAlert(StopAlertState.Issue, mapStopRoute)
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): I think we don't need to include the alert here after all, based on this alert case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would we want to just pass in null? I assume that would leave us without any indication of a disruption in the interim

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think null would be OK, but also totally fine to leave as-is for the interim state since this will be refined in the upcoming disruption on a branch task

@BrandonTR BrandonTR added this pull request to the merge queue Apr 17, 2025
Merged via the queue into main with commit 9a211cc Apr 17, 2025
10 checks passed
@BrandonTR BrandonTR deleted the br-group-by-direction-alerts branch April 17, 2025 14:53
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
0