Skip to content
This repository was archived by the owner on Oct 4, 2023. It is now read-only.

[C-422] Add pull to refresh on android#1275

Merged
sliptype merged 1 commit into
mainfrom
sk-c-422-android-pull-to-refresh
May 2, 2022
Merged

[C-422] Add pull to refresh on android#1275
sliptype merged 1 commit into
mainfrom
sk-c-422-android-pull-to-refresh

Conversation

@sliptype

@sliptype sliptype commented May 2, 2022

Copy link
Copy Markdown
Contributor

Description

  • Adds pull to refresh on android

Unfortunately, android does not support "overscrolling" or scrolling past the ends of a scrollview (contrary to what this prob would lead you to believe https://reactnative.dev/docs/scrollview#overscrollmode-android). So there is no way to use the same refresh indicator as we do on ios. Instead, we can use the native android one which looks a lot better than on ios

The only strange part of this is on the collapsible tabs on the profile screen. The indicator shows at the top of the list instead of at the top of the screen, which looks a little weird when scrolling down while the indicator is visible. But I think probably the best we can do for now because when shown at the top, the header covers the indicator and there seems to be no way of bringing the indicator to the front

22-05-02-15-23-32.mp4

Dragons

Possible pull to refresh bugs

How Has This Been Tested?

android and ios

How will this change be monitored?

Testflight / playstore

@sliptype sliptype requested a review from Kyle-Shanks May 2, 2022 20:31

@dylanjeffers dylanjeffers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great on lists at the top of the screen! regarding your issue with the profile... is there any way to add a refreshcontrol to a normal view and trigger manually depending on scroll or something?

<RefreshControl
progressViewOffset={scrollPropsAndRef.progressViewOffset}
refreshing={!!refreshing}
onRefresh={onRefresh ?? undefined}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i believe as long as onRefresh is undefined coming down don't need the ??. Is it null or something sometimes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah it has a null in there so typescript was angry

refreshing={!!isRefreshing}
onRefresh={onRefresh ?? undefined}
colors={[neutral]}
/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

kinda a nit, but maybe we make this a base component that all the lists can use? handles color, maybe also platform etc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That was my first approach, but for some reason having anything other than RefreshControl at the top level of this value breaks the entire flatlist. Might have to do with how it's interfacing with native components. This code is necessarily WET

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh woww interesting! well thanks for the context

@sliptype

sliptype commented May 2, 2022

Copy link
Copy Markdown
Contributor Author

Looks great on lists at the top of the screen! regarding your issue with the profile... is there any way to add a refreshcontrol to a normal view and trigger manually depending on scroll or something?

Unfortunately there is no way to add RefreshControl outside of a scroll view, we would have to roll our own which would probably require designs because currently the refresh indicators aren't displayed on top of other components (except the cover photo). I think what I have should be good for a first android release and we can maybe do another pass at it!

@dylanjeffers

Copy link
Copy Markdown
Contributor

Looks great on lists at the top of the screen! regarding your issue with the profile... is there any way to add a refreshcontrol to a normal view and trigger manually depending on scroll or something?

Unfortunately there is no way to add RefreshControl outside of a scroll view, we would have to roll our own which would probably require designs because currently the refresh indicators aren't displayed on top of other components (except the cover photo). I think what I have should be good for a first android release and we can maybe do another pass at it!

Thank you for helping me realize all the limitations with this api, this looks like a great first step

@sliptype sliptype merged commit 674ca18 into main May 2, 2022
@sliptype sliptype deleted the sk-c-422-android-pull-to-refresh branch May 2, 2022 22:08
@sliptype sliptype mentioned this pull request May 13, 2022
@AudiusProject AudiusProject deleted a comment from linear Bot Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants