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

[Customer Center]: fix navigation when embedded in NavigationStack #4622

Merged
merged 6 commits into from
Jan 14, 2025

Conversation

aboedo
Copy link
Member

@aboedo aboedo commented Dec 27, 2024

Fixes the navigation issues for Customer Center when presented through a NavigationStack.

Solves the issue by adding an extra optional parameter that you use when showing CustomerCenterView in a NavigationStack:

    CustomerCenterView(isInNavigationStack: true)
Before After
https://github.com/user-attachments/assets/68154d11-8a82-43e0-95ec-482c0c40446d https://github.com/user-attachments/assets/034b1b88-a0ef-4e90-9d4c-cec4cddaa72b

The Problem:

When presenting the Customer Center in a navigation stack, if we add another one, we kinda break navigation. I reproduced this in a very, very simple SwiftUI app:
image

What happens is that you go into customer center, it flashes but then automatically dismisses as soon as it loads, because the internal navigation stack and the external one kinda fight each other. If you try to tap again, it errors out.

The Solution

The solution is actually dead simple - if we're already in a NavigationStack, we don't create a new one.

What I don't love about this solution is that we do kind of give the consumer the responsibility of passing in this parameter correctly, but I tried my best to find ways around it to auto-detect and couldn't come up with something reliable.

And it doesn't feel that complicated of a thing to pass in.

Still to be done:

  • API Tests
  • Do we need to update the presentation modes? Do we want to reuse presentation modes? I kinda tried but it didn't fit all that well since it's meant for sheets, but maybe it can be fitted better.
  • Do we need to update events / metrics for this?

@aboedo aboedo self-assigned this Dec 27, 2024
@aboedo aboedo requested a review from vegaro December 27, 2024 19:02
@aboedo aboedo changed the title fixed navigation with new param Customer Center: fix navigation when embedded in NavigationStack Dec 27, 2024
@aboedo aboedo changed the title Customer Center: fix navigation when embedded in NavigationStack [Customer Center] fix navigation when embedded in NavigationStack Dec 27, 2024
@aboedo aboedo changed the title [Customer Center] fix navigation when embedded in NavigationStack [Customer Center]: fix navigation when embedded in NavigationStack Dec 27, 2024
@aboedo
Copy link
Member Author

aboedo commented Dec 27, 2024

@vegaro hope this is helpful! Can you take it from here to the finish line if it is?

@aboedo
Copy link
Member Author

aboedo commented Jan 9, 2025

cc @ajpallares @facumenzella

@hiddevdploeg
Copy link

Personally I'd prefer something like this:

NavigationLink("Customer Center") {
     CustomerCenterView()
}

ContentView()
.sheet(isPresented: $presentCustomerCenter) {
     CustomerCenterSheetView()
}

But I understand this would mean a everyone right now would have to refactor their current implementation so maybe CustomerCenterNavigationView() would make more sense?

Or we would create our own NavigationLink that looks something like this:

CustomerCenterNavigationLink {
     // Label goes here
}

Just my 2cts 😄

@facumenzella
Copy link
Contributor

@hiddevdploeg I'll open a follow-up PR after this one addressing your comments. I like it

@facumenzella facumenzella enabled auto-merge (squash) January 14, 2025 13:23
@facumenzella facumenzella disabled auto-merge January 14, 2025 16:10
@facumenzella facumenzella enabled auto-merge (squash) January 14, 2025 16:14
@facumenzella facumenzella merged commit 2d5bc58 into main Jan 14, 2025
9 of 10 checks passed
@facumenzella facumenzella deleted the andy/customer_center_fix_swiftui_navigation branch January 14, 2025 16:26
@tonidero
Copy link
Contributor

Just a heads up that I'm adding the pr:RevenueCatUI label so the changelog gets generated correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants