search

hotwired / turbo-ios

iOS framework for making Turbo native apps
MIT License
874 stars 88 forks source link

Do not explicitly set the constraints for refreshControl #202

Open pfeiffer opened 4 months ago

pfeiffer commented 4 months ago

Today, the refresh control is installed with layout constraints explicitly set. The explicitly set layout constraints has no visible effect vs. just not setting them, but setting them does cause imcompatibility with any custom consetInsets of the scroll view.

If for example a VisitableView is shown in an app with a translucent navigation bar, the refresh control will - with explicit layout constraints - be positioned under the tranlucent navigation bar, even if the scroll view has it's contentInset configured to correspond to the navigation bar height.

Since the installRefreshControl method is marked as private, it's not possible to easily skip adding the layout constraints today to make the refresh control compatible with custom content insets.

This PR removes the explicit layout constraints.

In the demo app, there's no visible difference between setting them explicitly and not setting them and the existing behavior. This is a video of the demo app with this PR applied:

https://github.com/hotwired/turbo-ios/assets/195925/1f73da10-f788-4130-abb6-f64aa08ef544

pfeiffer commented 4 months ago

In the original commit https://github.com/hotwired/turbo-ios/commit/a9c19d7b66b8079c04f9cb8fbaaf38c2a502b7ac by @svara it doesn't mention why the constaints are added, so it could be that I've missed the reason why they are there?

joemasilotti commented 4 months ago

I'm all for this change if it doesn't have any negative effects to existing apps. But I'm curious of the problem you're describing - can you record a video or provide code to run of the refresh control under the navigation bar?

svara commented 4 months ago

@pfeiffer Explicit constraints were introduced in https://github.com/hotwired/turbo-ios/pull/123 for the following reason: the conventional approach of assigning the UIRefreshControl directly to the web view's scroll view did not provide the desired functionality when using viewport-fit=cover. See https://github.com/hotwired/turbo-ios/pull/73 for more details.

I can see how the current approach doesn't respect custom content insets though. I'll take a look at this next week.

svara commented 4 months ago

@pfeiffer I’m not able to reproduce the refresh control under the navigation bar issue. In my tests, it is always correctly presented under the navigation bar, whether it is transparent or not. As for the viewport, I tested with and without viewport-fit=cover. This is the code I used to configure a transparent navigation bar:

let appearance = UINavigationBarAppearance()
appearance.configureWithTransparentBackground()
UINavigationBar.appearance().standardAppearance = appearance

Regarding custom insets, we can address the issue by providing the top anchor with the scroll view's top content inset.

refreshControl.topAnchor.constraint(equalTo: safeAreaLayoutGuide.topAnchor, constant: scrollView.contentInset.top)