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

Fix refresh control positioning #123

Merged
merged 3 commits into from
May 30, 2023
Merged

Fix refresh control positioning #123

merged 3 commits into from
May 30, 2023

Conversation

svara
Copy link
Contributor

@svara svara commented May 16, 2023

Summary

This PR fixes refresh control's positioning by using constraints.

Context

UIRefreshControl is a critical component that enables users to refresh content by pulling down on a scroll view.
The conventional approach involves assigning the UIRefreshControl directly to the web view's scroll view. However, in a specific case (using viewport-fit=cover), this approach did not provide the desired functionality. See #73.

Solution

To address this issue, we have updated the UIRefreshControl positioning by utilizing constraints. With this approach, we can ensure the UIRefreshControl adjusts properly within the safe area.

@svara svara changed the title Update refresh control handling Update refresh control positioning May 19, 2023
@svara svara changed the title Update refresh control positioning Fix refresh control positioning May 19, 2023
@svara svara requested review from olivaresf and jayohms May 19, 2023 15:02
@svara
Copy link
Contributor Author

svara commented May 19, 2023

@spinosa @ghiculescu @joemasilotti @Bramjetten I saw you guys were involved in the related PR (#73).
The proposed solution works in our case with viewport-fit=cover and should cover all cases, but I'd like to ask you for feedback/testing. Getting some more eyes on the solution would be great, so we don't break existing apps.
Thank you!

@svara svara marked this pull request as ready for review May 19, 2023 15:16
Comment on lines +72 to +76

/// Infer refresh control's default height from its frame, if given.
/// Otherwise fallback to 60 (the default height).
let refreshControlHeight = refreshControl.frame.height > 0 ? refreshControl.frame.height : 60

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to specify the height?
Internally, refresh control doesn't use autolayout for positioning its subviews. Its main frame doesn't reflect its true size. The main container height is 60pt, but the view where the refresh image/animation is has a height of 100pt. When using autolayout the latter is used. 🤷‍♂️ Even if we don't set the height, the refresh control will respect the safe area, but it won't be positioned as it should be.

Screenshot 2023-05-17 at 11 52 53

Comment on lines +77 to +81
NSLayoutConstraint.activate([
refreshControl.centerXAnchor.constraint(equalTo: centerXAnchor),
refreshControl.topAnchor.constraint(equalTo: safeAreaLayoutGuide.topAnchor),
refreshControl.heightAnchor.constraint(equalToConstant: refreshControlHeight)
])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using autolayout ensures the system takes care of updating the view's position as appropriate.
centerXAnchor makes sure the control is always centered with the view’s horizontal center.
Pinning control's top anchor to the view's safe area top anchor ensures the control is positioned within the safe area.

@Bramjetten
Copy link
Contributor

@spinosa @ghiculescu @joemasilotti @Bramjetten I saw you guys were involved in the related PR (#73).
The proposed solution works in our case with viewport-fit=cover and should cover all cases, but I'd like to ask you for feedback/testing. Getting some more eyes on the solution would be great, so we don't break existing apps.
Thank you!

Thanks for this! Will try it out in our apps next week and report back 👍

@Bramjetten
Copy link
Contributor

I have just tested and confirmed it works in my project! Thanks, this is much appreciated!

@svara
Copy link
Contributor Author

svara commented May 23, 2023

That's great to hear @Bramjetten! Thanks!

@svara svara merged commit 293150b into main May 30, 2023
1 check passed
@svara svara deleted the refresh-control-fix branch May 30, 2023 14:28
@svara svara mentioned this pull request Feb 26, 2024
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.

None yet

2 participants