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

Do not explicitly set the constraints for refreshControl #202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pfeiffer
Copy link
Contributor

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:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-22.at.09.22.06.mp4

@pfeiffer
Copy link
Contributor Author

In the original commit a9c19d7 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
Copy link
Member

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
Copy link
Contributor

svara commented May 3, 2024

@pfeiffer Explicit constraints were introduced in #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 #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
Copy link
Contributor

svara commented May 20, 2024

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants