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.

Sign up for GitHub

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
+159 −121
Open

feat: Update stop details navigation to match spec #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
236aa02
feat: Update stop details navigation to match spec
EmmaSimon Dec 12, 2024
ea2142e
test: Add tests for stop details nav behavior
EmmaSimon Dec 13, 2024
5cc0369
test: Remove unfiltered back button tests
EmmaSimon Dec 13, 2024
022f89b
Merge branch 'main' into es-combined-stop-nav
EmmaSimon Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion iosApp/iosApp/Pages/StopDetails/StopDetailsFilteredView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ struct StopDetailsFilteredView: View {
stop: stop,
pinned: pinned,
onPin: toggledPinnedRoute,
onClose: { nearbyVM.navigationStack.removeAll() }
onClose: { nearbyVM.goBack() }
)
if nearbyVM.showDebugMessages {
DebugView {
Expand Down
1 change: 0 additions & 1 deletion iosApp/iosApp/Pages/StopDetails/StopDetailsUnfilteredView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ struct StopDetailsUnfilteredView: View {
VStack(spacing: 16) {
SheetHeader(
title: stop?.name ?? "Invalid Stop",
onBack: nearbyVM.navigationStack.count > 1 ? { nearbyVM.goBack() } : nil,
onClose: { nearbyVM.navigationStack.removeAll() }
)
if nearbyVM.showDebugMessages {
Expand Down
25 changes: 23 additions & 2 deletions iosApp/iosApp/Utils/SheetNavigationStackEntry.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,29 @@ extension [SheetNavigationStackEntry] {
if case let .legacyStopDetails(stop, _) = self.last {
_ = self.popLast()
self.append(.legacyStopDetails(stop, newValue))
} else if case let .stopDetails(stopId: stopId, stopFilter: _, tripFilter: _) = self.last {
_ = self.popLast()
} else if case let .stopDetails(
stopId: stopId,
stopFilter: lastFilter,
tripFilter: _
) = self.last {
// When the stop filter changes, we want a new entry to be added (i.e. no pop) only when
// you're on the unfiltered (lastFilter == nil) page, but if there is already a filter,
// the entry with the old filter should be popped and replaced with the new value.
if lastFilter != nil { _ = self.popLast() }

// If the new value 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
if newValue == nil,
case let .stopDetails(
stopId: penultimateStop,
stopFilter: penultimateFilter,
tripFilter: _
) = self.last,
penultimateStop == stopId,
penultimateFilter == nil {
return
}

self.append(.stopDetails(stopId: stopId, stopFilter: newValue, tripFilter: nil))
}
}
Expand Down
36 changes: 31 additions & 5 deletions iosApp/iosApp/ViewModels/NearbyViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,40 @@ class NearbyViewModel: ObservableObject {
)
pushNavEntry(.stopDetails(stopId: stopId, stopFilter: stopFilter, tripFilter: tripFilter))
} else if case let .legacyStopDetails(targetStop, _) = entry,
case let .legacyStopDetails(currentStop, _) = navigationStack.last,
targetStop == currentStop {
case let .legacyStopDetails(lastStop, _) = navigationStack.last,
targetStop == lastStop {
_ = navigationStack.popLast()
navigationStack.append(entry)
} else if case let .stopDetails(stopId: targetStop, stopFilter: _, tripFilter: _) = entry,
case let .stopDetails(stopId: currentStop, stopFilter: _, tripFilter: _) = navigationStack.last,
targetStop == currentStop {
} else if
case let .stopDetails(
stopId: targetStop,
stopFilter: newFilter,
tripFilter: _
) = entry,
case let .stopDetails(
stopId: lastStop,
stopFilter: lastFilter,
tripFilter: _
) = navigationStack.last,
lastFilter != nil,
targetStop == lastStop {
// When the stop filter changes, we want a new entry to be added (i.e. no pop) only when
// you're on the unfiltered (lastFilter == nil) page, but if there is already a filter,
// the entry with the old filter should be popped and replaced with the new value.
_ = navigationStack.popLast()

// 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
Comment on lines +141 to +142
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.)

if newFilter == nil,
case let .stopDetails(
stopId: penultimateStop,
stopFilter: penultimateFilter,
tripFilter: _
) = navigationStack.last,
penultimateStop == targetStop,
penultimateFilter == nil {
return
}
navigationStack.append(entry)
} else {
navigationStack.append(entry)
Expand Down
45 changes: 0 additions & 45 deletions iosApp/iosAppTests/Pages/StopDetails/StopDetailsPageTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,51 +115,6 @@ final class StopDetailsPageTests: XCTestCase {
wait(for: [exp], timeout: 30)
}

func testBackButton() throws {
let objects = ObjectCollectionBuilder()
let stop = objects.stop { _ in }

class FakeNearbyVM: NearbyViewModel {
let backExp: XCTestExpectation
init(_ backExp: XCTestExpectation) {
self.backExp = backExp
super.init(combinedStopAndTrip: true)
}

override func goBack() {
backExp.fulfill()
}
}

let backExp = XCTestExpectation(description: "goBack called")
let nearbyVM = FakeNearbyVM(backExp)
nearbyVM.navigationStack = [
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
]

let stopDetailsVM = StopDetailsViewModel(
predictionsRepository: MockPredictionsRepository(),
schedulesRepository: MockScheduleRepository()
)

let sut = StopDetailsPage(
stopId: stop.id,
stopFilter: nil,
tripFilter: nil,
errorBannerVM: .init(),
nearbyVM: nearbyVM,
mapVM: .init(),
stopDetailsVM: stopDetailsVM,
viewportProvider: .init()
)

try sut.inspect().find(viewWithAccessibilityLabel: "Back").button().tap()

wait(for: [backExp], timeout: 2)
}

func testCloseButton() throws {
let objects = ObjectCollectionBuilder()
let stop = objects.stop { _ in }
Expand Down
54 changes: 0 additions & 54 deletions iosApp/iosAppTests/Pages/StopDetails/StopDetailsViewTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,60 +145,6 @@ final class StopDetailsViewTests: XCTestCase {
XCTAssert(nearbyVM.navigationStack.isEmpty)
}

func testBackButtonGoesBack() throws {
let objects = ObjectCollectionBuilder()
let stop = objects.stop { _ in }

let initialNavStack: [SheetNavigationStackEntry] = [
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
.tripDetails(tripId: "", vehicleId: "", target: nil, routeId: "", directionId: 0),
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
]
let nearbyVM: NearbyViewModel = .init(navigationStack: initialNavStack)
let sut = StopDetailsView(
stopId: stop.id,
stopFilter: nil,
tripFilter: nil,
setStopFilter: { _ in },
setTripFilter: { _ in },
departures: nil,
now: Date.now,
errorBannerVM: .init(),
nearbyVM: nearbyVM,
mapVM: .init(),
stopDetailsVM: .init()
)

ViewHosting.host(view: sut)
try? sut.inspect().find(viewWithAccessibilityLabel: "Back").button().tap()
XCTAssertEqual(initialNavStack.dropLast(), nearbyVM.navigationStack)
}

func testBackButtonHiddenWhenNearbyIsBack() throws {
let objects = ObjectCollectionBuilder()
let stop = objects.stop { _ in }

let nearbyVM: NearbyViewModel = .init(navigationStack: [
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
])
let sut = StopDetailsView(
stopId: stop.id,
stopFilter: nil,
tripFilter: nil,
setStopFilter: { _ in },
setTripFilter: { _ in },
departures: nil,
now: Date.now,
errorBannerVM: .init(),
nearbyVM: nearbyVM,
mapVM: .init(),
stopDetailsVM: .init()
)

ViewHosting.host(view: sut)
XCTAssertNil(try? sut.inspect().find(viewWithAccessibilityLabel: "Back"))
}

func testDebugModeNotShownByDefault() throws {
let objects = ObjectCollectionBuilder()
let stop = objects.stop { stop in
Expand Down
41 changes: 37 additions & 4 deletions iosApp/iosAppTests/Utils/SheetNavigationStackEntryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ final class SheetNavigationStackEntryTests: XCTestCase {
XCTAssertEqual(stack, [])
}

func testLastFilterShallow() throws {
func testLastFilterShallowLegacy() throws {
let stop = ObjectCollectionBuilder.Single.shared.stop { _ in }
var stack: [SheetNavigationStackEntry] = [.legacyStopDetails(stop, nil)]

Expand All @@ -38,7 +38,7 @@ final class SheetNavigationStackEntryTests: XCTestCase {
XCTAssertEqual(stack.lastStopDetailsFilter, nil)
}

func testLastFilterDeep() throws {
func testLastFilterDeepLegacy() throws {
let stop = ObjectCollectionBuilder.Single.shared.stop { _ in }
let otherStop = ObjectCollectionBuilder.Single.shared.stop { _ in }
let previousEntries: [SheetNavigationStackEntry] = [
Expand Down Expand Up @@ -67,7 +67,7 @@ final class SheetNavigationStackEntryTests: XCTestCase {
XCTAssertEqual(stack.lastStopDetailsFilter, nil)
}

func testLastFilterNotTop() throws {
func testLastFilterNotTopLegacy() throws {
let stop = ObjectCollectionBuilder.Single.shared.stop { _ in }
var stack: [SheetNavigationStackEntry] = [
.legacyStopDetails(stop, .init(routeId: "A", directionId: 1)),
Expand Down Expand Up @@ -108,7 +108,7 @@ final class SheetNavigationStackEntryTests: XCTestCase {
XCTAssertEqual(alertEntry.coverItemIdentifiable()!.id, "0")
}

func testNavStackLastStop() throws {
func testNavStackLastStopLegacy() throws {
let stop = ObjectCollectionBuilder.Single.shared.stop { _ in }
let stopEntry: SheetNavigationStackEntry = .legacyStopDetails(stop, .init(routeId: "A", directionId: 1))
let tripEntry: SheetNavigationStackEntry = .tripDetails(
Expand Down Expand Up @@ -166,4 +166,37 @@ final class SheetNavigationStackEntryTests: XCTestCase {
stack.append(tripEntry)
XCTAssertEqual(stop.id, stack.lastStopId)
}

func testLastFilterSet() throws {
let stop = ObjectCollectionBuilder.Single.shared.stop { _ in }
var stack: [SheetNavigationStackEntry] = [.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil)]

let filter1 = StopDetailsFilter(routeId: "A", directionId: 1)
stack.lastStopDetailsFilter = filter1

XCTAssertEqual([
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
.stopDetails(stopId: stop.id, stopFilter: filter1, tripFilter: nil),
], stack)
XCTAssertEqual(filter1, stack.lastStopDetailsFilter)

let filter2 = StopDetailsFilter(routeId: "A", directionId: 0)
stack.lastStopDetailsFilter = filter2

XCTAssertEqual([
.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil),
.stopDetails(stopId: stop.id, stopFilter: filter2, tripFilter: nil),
], stack)
XCTAssertEqual(filter2, stack.lastStopDetailsFilter)

stack.lastStopDetailsFilter = nil

XCTAssertEqual([.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil)], stack)
XCTAssertEqual(nil, stack.lastStopDetailsFilter)

stack.lastStopDetailsFilter = nil

XCTAssertEqual([.stopDetails(stopId: stop.id, stopFilter: nil, tripFilter: nil)], stack)
XCTAssertEqual(nil, stack.lastStopDetailsFilter)
}
}
Loading
Loading