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: Update stop details navigation to match spec #586

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

EmmaSimon
Copy link
Contributor

Summary

Ticket: Combined Stop + Trip Details: nav changes

This updates the combined stop/trip navigation to match the desired behavior. Back buttons are removed from the filtered and unflitered stop pages, and the X button on filtered stop details will bring you back to the place you came from, either the unfiltered stop or nearby transit. Because this doesn't involve changing the actual entry type, but just the stop filter on the entry, there's some slightly convoluted logic to ensure that filter entries are appended rather than replacing the unfiltered entry, and that when the filtered entries are removed, that they don't add duplicate unfiltered entries. This logic also unfortunately needs to exist in two places, since a new entry can be pushed in its entirety to the nearby VM, or the filter value can be modified independently through lastStopDetailsFilter.

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

Testing

Added tests for new behavior

@EmmaSimon EmmaSimon requested a review from a team as a code owner December 13, 2024 00:38
Comment on lines +141 to +142
// If the new filter is nil and there is already a nil filter in the stack for the same stop ID,
// we don't want a duplicate unfiltered entry, so skip appending a new one
Copy link
Member

Choose a reason for hiding this comment

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

Should this condition really be inside the lastFilter != nil? It isn't in the other copy of this logic. (It would probably be worth refactoring this logic out somewhere else even if the result is a bit ugly at each call site.)

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