[feat]: add support for sizingOptions to SwiftUIScreen #277
Conversation
n8chur
changed the title
n8chur
changed the title
| setNeedsLayoutBeforeFirstLayoutIfNeeded() | ||
| } | ||
|
|
||
| override func viewDidLayoutSubviews() { |
There was a problem hiding this comment.
Market also supports having the preferredContentSize automatically calculated if it's requested by a consumer via an associated value on UIViewController—we'll likely want to figure out a way to support this with SwiftUIScreens, and that may involve moving some of that infra into the Workflow repo (UI-5801).
Hmm yeah this is an interesting one, since that stuff all lives in Modals right now. We'll definitely want to keep it, since avoiding the duplicative preferredContentSize calculations was a pretty big performance win.
Trying to think if there's a way we could flow this into the SwiftUI environment at the Market layer... But I guess since this is a view controller, we don't have that available.
One gross idea, but it would work, is do a respondsToSelector check on the associated value we added, and reference it via valueForKey in this repo. Icky but it would do the thing we want...
There was a problem hiding this comment.
I think we may want to consider moving that associated object to a ViewEnvironmentKey, and then move that key's definition to ViewEnvironmentUI, possibly also adding support for this automatically in WorkflowUI (along with here).
Let's move further discussion on this to UI-5801 so that the context is easier to discover once we start that work!
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
|
|
||
| view.backgroundColor = .clear |
There was a problem hiding this comment.
Just curious: is this new and necessary for the PCS behavior we want?
There was a problem hiding this comment.
Oh yeah, whoops, meant to call this out or pull it into a separate PR...
It's not necessary to achieve the goal of this PR, but IMO it's generally expected, and annoying to work around if you want to have a transparent background when working with Screens like this. We also set this in WorkflowHostingController.
Epoxy sets this on their hosting controller and they included a comment describing their motivation:
// A `UIHostingController` has a system background color by default as it's typically used in
// full-screen use cases. Since we're using this view controller to place SwiftUI views within
// other view controllers we default the background color to clear so we can see the views
// below, e.g. to draw highlight states in a `CollectionView`.Happy to move this to a separate PR if we'd prefer!
There was a problem hiding this comment.
Thanks for explaining. Makes sense to me, and not important to break out IMO 👍
There was a problem hiding this comment.
Maybe it's worth cribbing that comment into our implementation too.
There was a problem hiding this comment.
worth adding a similar comment here?
Eh, maybe? I think it's probably pretty self-explanatory.
Happy to add one if anyone feels strongly though!
There was a problem hiding this comment.
Maybe it's worth cribbing that comment into our implementation too.
2 comments about it seems like a signal that I should add the comment—I'll do so in a follow up!
|
Merging now that I have 2 stamps but more than happy to follow up with additional changes if requested! |
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
|
|
||
| view.backgroundColor = .clear |
There was a problem hiding this comment.
Maybe it's worth cribbing that comment into our implementation too.
Market's Modal framework relies on
preferredContentSizeto size modals during presentation.Without any changes, view controllers that back
SwiftUIScreens never had theirpreferredContentSizeupdated.SwiftUIScreenis backed by aUIHostingControllersubclass. In iOS 16 and above, a newsizingOptionparameter was added that allows a consumer to specify the.preferredContentSizeoption which will cause the view controller to update thepreferredContentSizeas it changes over time. Before iOS 16, you have to do this manually.One thing that's surprising is that with the
sizingOptionsupport forpreferredContentSize, thepreferredContentSizedoes not appear to be updated after the first layout. Also, theUIHostingController'sviewis does not appear to havesetNeedsLayoutset, which has generally been expected in Market's infrastructure so far. I've opened UI-5797 to investigate this to see whether Market's Modal framework is holding this wrong, or if the workarounds found in this PR are in fact needed.Market also supports having the
preferredContentSizeautomatically calculated if it's requested by a consumer via an associated value onUIViewController—we'll likely want to figure out a way to support this withSwiftUIScreens, and that may involve moving some of that infra into the Workflow repo (UI-5801).I've also introduced a test target for
WorkflowSwiftUIExperimentalso that I could add some tests related topreferredContentSizecalculations.An integration experiment was added to Market in here.
These changes resolve UI-5766.
Checklist