Skip to content

Add onPresentation handler to present methods - #356

Open
jshier wants to merge 3 commits into
pointfreeco:mainfrom
jshier:presentation-onPresent
Open

Add onPresentation handler to present methods#356
jshier wants to merge 3 commits into
pointfreeco:mainfrom
jshier:presentation-onPresent

Conversation

@jshier

@jshier jshier commented Jun 24, 2026

Copy link
Copy Markdown

This PR adds an onPresentation closure to be called when UIKit presentation completes, using the standard completion parameter.

This is a first pass at a simple integration of this feature, as there are a few unresolved questions.

  1. Are unit tests needed here? I see that onDismiss doesn't have any tests, just example calls, so that's what I matched.
  2. Should there be a version of this API that passes the ID of the presentation? It might be useful for things like TCA, where callers may want to take different actions on presentation and dismissal depending on the CaseScope.
  3. I implemented this at the present level since that's where onDimiss is implemented, and because that's where UIKit's present is called. Is there any desire to move this closure and onDimiss down to destination in some way?

@jshier

jshier commented Jun 26, 2026

Copy link
Copy Markdown
Author

If possible, I'd appreciate a look at this soon, as I'd like to use it in my codebase to replace some custom presentation logic, especially since we already use swift-navigation in other places. Any thoughts on this approach or feature in general?

@stephencelis

Copy link
Copy Markdown
Member

@jshier Haven't had time to dig in yet but from our Slack convo it sounded reasonable. We have a lot of other items we are working through at the moment so we don't know when we'll get to this, but you should be able to point to your fork in the meantime.

@jshier

jshier commented Jun 26, 2026

Copy link
Copy Markdown
Author

Unfortunately that would require forking every library that depends on swift-navigation, as SPM uses the URL for identity, so my fork won't replace the library dynamically. That may only be TCA, but I can't ship a double forked change for this. If you won't get to this for a while I can ship a poor workaround in the meantime, when I need to.

@jshier

jshier commented Jun 29, 2026

Copy link
Copy Markdown
Author

In the meantime, anything I can do to better prove the change here? Any test scenarios to add to the example app?

@stephencelis

Copy link
Copy Markdown
Member

@jshier It'd be nice to get some coverage in the Examples/CaseStudies test target. There are a bunch of end-to-end tests there and I think a few for onPresentation would be nice. And we'll try to review this soon!

@jshier

jshier commented Jul 3, 2026

Copy link
Copy Markdown
Author

I added a few tests, let me know if there are any additional specific cases you'd like covered.

I've also been thinking about my underlying need here, which is an onPresentation callback from TCA-powered presentations that receives the case of the presented destination. Due to content possibly being called multiple times before an onPresentation, to achieve that with the current API I'll have to accumulate the cases seen in content and assume the last will be the one presented, then clear that state in onDismiss. I just wonder if there might be a more reliable solution that passes the ID through these APIs instead, but I don't know if the case is the ID here. Any thoughts?

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.

2 participants