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

Add automatic RefreshControl offset adjustment behavior #281

Merged

Conversation

alexisakers
Copy link
Contributor

Goal

When RefreshControl.isRefreshing becomes true because of a change of state that is not caused by the user pulling to refresh (for example changing filters or tapping retry from an error cell), we want to be able to display the refresh control to indicate that loading is in progress.

Implementation Details

  • We add an offsetAdjustmentBehavior property to RefreshControl to allow consumers to opt into this behavior
  • Create a adjustContentOffsetForRefreshControl method in PresentationState to adjust the offset based on the current state of the refresh control
  • Call that method from updatePresentationState in ListView after content updates and before performing any auto scroll actions

Demo

iOS 12

auto.refresh.control.ios.12.mov

iOS 13

auto.refresh.control.ios.13.mov

iOS 14

auto.refresh.control.ios.14.mov

switch control.model.offsetAdjustmentBehavior {
case .displayWhenRefreshing(let animate):
let contentOffset = CGPoint(x: 0, y: -control.view.frame.height - view.safeAreaInsets.top)
view.setContentOffset(contentOffset, animated: animate)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As modeled right now, it'll always scroll to the top of the list when refreshing begins, right? Is that desired?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Eg, seems like maybe it should only become visible if the list is already at the top?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is more of a product/design question I feel like, but this is generally why I like to avoid using the refresh control to indicate a programmatic refresh. Perhaps this should also be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the feature that prompted this change, we do want to scroll to the top when we start refreshing through a workflow action, but it's a good point, I also think it should be configurable.

Would you rather add a scrollToTop associated value to displayWhenRefreshing, or have a new scrollToTopWhenRefreshing case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a huge preference, an associated scrollToTop makes sense to me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be great to update the demo for this case too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kylebshr added the parameter and updated the demo!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Kyle Bashour <kylebshr@me.com>
@kylebshr kylebshr merged commit c08d33d into square:main Mar 22, 2021
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.

None yet

3 participants