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] Custom sheet auto dismiss #306

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

CalebRas
Copy link
Contributor

Description

This PR implements fixes to the following bugs that would occur when using the custom sheet modifier in the sample viewer:

  1. If there was more than one custom sheet within a given view, some of the sheets would auto-dismiss upon opening, e.g. a sample containing settings sheet would cause the info sheet to auto-dismiss when opened.
  2. Whenever a custom sheet was closed, Modifying state during view update, this will cause undefined behavior. would be thrown.
  3. Custom sheets dismissed using the DismissAction from the Environment wouldn't always re-present.

URL to custom sheet modifier file: View+Sheet.swift

Linked Issue(s)

  • swift/issues/4778

How To Test

Test Code
struct DisplayMapView: View {
    /// A map with imagery basemap.
    @State private var map = Map(basemapStyle: .arcGISImagery)
    
    @State private var isShowingDoneSheet = false
    
    @State private var isShowingDragSheet = false
    
    @State private var isShowingDismissSheet = false
    
    var body: some View {
        // Creates a map view to display the map.
        MapView(map: map)
            .toolbar {
                ToolbarItemGroup(placement: .bottomBar) {
                    Button("Plain") {
                        isShowingDragSheet = true
                    }
                    .sheet(isPresented: $isShowingDragSheet, detents: [.medium, .large], dragIndicatorVisibility: .visible) {
                        print("Dismissed")
                    } content: {
                        Text("Some text.")
                    }
                    Spacer()
                    
                    Button("Done") {
                        isShowingDoneSheet = true
                    }
                    .sheet(isPresented: $isShowingDoneSheet, detents: [.medium]) {
                        print("Dismissed")
                    } content: {
                        NavigationView {
                            Text("Press Done.")
                                .toolbar {
                                    ToolbarItem(placement: .confirmationAction) {
                                        Button("Done") {
                                            isShowingDoneSheet = false
                                        }
                                    }
                                }
                        }
                    }
                    Spacer()
                    
                    Button("Dismiss") {
                        isShowingDismissSheet = true
                    }
                    .sheet(isPresented: $isShowingDismissSheet, detents: [.large]) {
                        print("Dismissed")
                    } content: {
                        DismissSheetView()
                    }
                }
            }
    }
}

private extension DisplayMapView {
    struct DismissSheetView: View {
        /// The action to dismiss the sheet.
        @Environment(\.dismiss) private var dismiss
        
        var body: some View {
            NavigationView {
                Text("Press dismiss")
                    .toolbar {
                        ToolbarItem(placement: .confirmationAction) {
                            Button("Dismiss") {
                                dismiss()
                            }
                        }
                    }
            }
        }
    }
}
  • Copy the code above into the Display map sample.
  • In the sample, verify the sheets open and close as expected using the buttons in the bottom toolbar.
  • Verify that info sheet (top-right button) works as expected.
  • Verify the rest of the samples using the custom sheet work as expected.

@CalebRas CalebRas self-assigned this Nov 29, 2023
@CalebRas CalebRas requested review from a team, njarecha and yo1995 and removed request for a team November 29, 2023 21:45
@CalebRas CalebRas changed the title Fix sheet auto dismiss. [Fix] Custom sheet auto dismiss Nov 29, 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.

Overall looks good! I'll spend a bit more time to take a closer look.

Shared/Supporting Files/Extensions/View+Sheet.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Extensions/View+Sheet.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@njarecha njarecha left a comment

Choose a reason for hiding this comment

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

This is just initial review. I'll continue on testing the fix.

Shared/Supporting Files/Extensions/View+Sheet.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Extensions/View+Sheet.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Extensions/View+Sheet.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Extensions/View+Sheet.swift Outdated Show resolved Hide resolved
njarecha
njarecha previously approved these changes Dec 4, 2023
Copy link
Contributor

@njarecha njarecha left a comment

Choose a reason for hiding this comment

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

Changes looks good and improves for some use cases. We can continue looking for improving some other cases separately.

yo1995
yo1995 previously approved these changes Dec 6, 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.

I've run test on multiple iOS 15 to 17 iPhones and iPads. The change mitigates the sheet dismissal problem. Though there are still some minor bugs, I think it is good to get this in for now.

@CalebRas CalebRas requested a review from yo1995 December 7, 2023 19:50
@CalebRas
Copy link
Contributor Author

CalebRas commented Dec 7, 2023

Thank you everyone!

@CalebRas CalebRas merged commit 806ac9b into v.next Dec 7, 2023
1 check passed
@CalebRas CalebRas deleted the Caleb/Fix-SheetExtensionAutoDismiss branch December 7, 2023 22:39
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

6 participants