search

johnpatrickmorgan / FlowStacks

FlowStacks allows you to hoist SwiftUI navigation and presentation state into a Coordinator
MIT License
854 stars 66 forks source link

Expose the `onDismiss` closure when presenting a sheet. #15

Closed lionel-alves closed 2 years ago

lionel-alves commented 2 years ago

This PR: Add an optional onDismiss closure to the presentSheet method to pass it to the native sheet method instead of passing nil. This address the issue described here No way to know when a sheet is dismissed #14

johnpatrickmorgan commented 2 years ago

Thanks very much for raising this PR @lionel-alves! It looks great. I figured it would make sense to expose the same for cover presentations too, so I added a commit on top on this branch to add that.

My one concern is that Route is no longer a simple value type. So when conforming Route to Equatable or Codable, the onDismiss closure has to be ignored. I think perhaps that's OK, though maybe a disclaimer should be added to that effect.

lionel-alves commented 2 years ago

Thanks @johnpatrickmorgan! Agreed for adding that to cover as well. Regarding the Equatable, I didn't find where it is needed in the codebase but I think it is fine as well. Not related to this PR but If we want to differentiate two instance of route that have the same screen, Route should probably be Identifiable using a uuid.

johnpatrickmorgan commented 2 years ago

Great, I've added some documentation and merged. Thanks for contributing!