feat(Android): Downstream disruptions/service changes #696
Conversation
| @@ -205,4 +205,8 @@ | |||
| <string name="waiting_to_depart">Waiting to depart</string> | |||
| <string name="weather">Weather</string> | |||
| <string name="westbound">Westbound</string> | |||
| <string name="effect_ahead">%1$s ahead</string> | |||
There was a problem hiding this comment.
Suggestion: could you alphabetize these? I've been forgetting to do it myself, added a reminder to the PR template in #694
| import com.mbta.tid.mbta_app.model.Alert | ||
|
|
||
| @Composable | ||
| fun StopDetailsDownstreamAlert( |
There was a problem hiding this comment.
@BrandonTR this looks good for downstream alerts, but I'm not seeing the service change / unspecified stop behavior mentioned in the acceptance criteria that is covered in the iOS AlertCard implementation. I think the StopDetailsDowsntreamAlert is the component used in legacy stop details, but the linked AlertCard is what is used in combined stop details. Please follow up in the dev channel if you have any qs!
There was a problem hiding this comment.
I think I misunderstood the scope between this ticket and this ticket I did start doing the full AlertCard impl as part of this ticket but thought it was going out of scope, going ahead and pushing it here!
There was a problem hiding this comment.
That makes sense - these tickets were broken down based on how things were iterated on in iOS, but we probably could have broken it down more clearly for the fresh start in android where there wasn't an existing StopDetailsDownstreamAlert view that needed converting and we could go straight to the AlertCard. Sorry for the confusion! Please ask as scope boundary questions like this come up in the future!
| alerts.forEach { AlertCard(it) } | ||
| downstreamAlerts.forEach { AlertCard(it, AlertCardSpec.Downstream) } | ||
| if (showElevatorAccessibility) { | ||
| elevatorAlerts.forEach { AlertCard(it, AlertCardSpec.Elevator) } |
There was a problem hiding this comment.
question: I see on mbta.com that there is an elevator alert at Davis, but I'm not seeing it on stop details with the elevator accessibility feature turned on, but I'm not sure why since this looks like what i'd expect to see. Any thoughts about what the issue might be? We can split that out from this PR though if needed.
There was a problem hiding this comment.
Briefly looked into this, the alert from the API seems to have the right effect and activities but we're still seeing 0 elevator alerts passed into this view. Not sure why.
There was a problem hiding this comment.
Thanks for looking into it! Non-blocking for this PR, I think it is OK to fix separately.
There was a problem hiding this comment.
I'm in the middle of the elevator alerts UI changes, I'll wait to put up the PR for that until after this is merged, since to be finalized it needs to use the AlertCard, and I can fix this as part of that.
There was a problem hiding this comment.
Going to go ahead and rebase this PR and merge it to unblock you! I'll add the remaining suggested tests as a follow up PR with the rest of the "disruptions here" work.
| import org.junit.Rule | ||
| import org.junit.Test | ||
|
|
||
| class AlertCardTests { |
There was a problem hiding this comment.
suggestion: I think it would be worth copying the integration tests from StopDetailsFilteredDepartureDetailsTests as well that cover alerts cases
- testShowsSuspension
- testShowsDownstreamAlert
- testShowsElevatorAlert
| import com.mbta.tid.mbta_app.model.Alert | ||
|
|
||
| @Composable | ||
| fun StopDetailsDownstreamAlert( |
There was a problem hiding this comment.
That makes sense - these tickets were broken down based on how things were iterated on in iOS, but we probably could have broken it down more clearly for the fresh start in android where there wasn't an existing StopDetailsDownstreamAlert view that needed converting and we could go straight to the AlertCard. Sorry for the confusion! Please ask as scope boundary questions like this come up in the future!
BrandonTR
force-pushed
the
br-downstream-service-disruption
branch
from
1417d54 to
f58236c
Compare
Summary
Ticket: 🤖 | Stop + Trip Details | Show disruption downstream & handle service changes + unspecified stops
Add mini MVP card for downstream disruptions, service changes, and unspecified stops.
iOS
- [ ] If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?- [ ] Add temporary machine translations, marked "Needs Review"android
- [ ] Expensive calculations are run inwithContext(Dispatchers.Default)where possibleTesting
Added tests for the various states.