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

[Fix] Sample Viewer navigation bug #290

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Conversation

CalebRas
Copy link
Contributor

@CalebRas CalebRas commented Nov 1, 2023

Description

This PR implements a fix to bugs that would come from nested NavigationLinks (e.g. using one in the top view of a sample) on iOS 16+ on both iPad and iPhone.

Linked Issue(s)

  • swift/issues/NA (quick fix)

How To Test

  • Ensure the navigating through the Sample Viewer is same.

@CalebRas CalebRas self-assigned this Nov 1, 2023
Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

From this doc: https://developer.apple.com/documentation/swiftui/navigationsplitview

You can also embed a NavigationStack in a column. Tapping or clicking a NavigationLink that appears in an earlier column sets the view that the stack displays over its root view. Activating a link in the same column adds a view to the stack.

I think this change makes sense. When a sample is selected from the sidebar ("an earlier column"), the detail view shows the sample as the root view. When a navigation link is tapped in the sample itself, the child view is pushed onto the nav stack of the detail view.

I've skimmed through all samples and there wasn't one that shows a list as the root view, so this wasn't a problem before.

I'm a bit wary because I haven't found a similar usage on GitHub anywhere. Also since there are many kinds of sidebar-detail relationship that a split view supports, the doc isn't very clear for our specific usecase, where both the sidebar and detail view needs stack-based navigation.

Nonetheless, this change seems to work so let's try it out.

@CalebRas CalebRas merged commit 8219818 into v.next Nov 2, 2023
1 check passed
@CalebRas CalebRas deleted the Caleb/Fix-NavigationBug branch November 2, 2023 16:25
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.

None yet

3 participants