react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Inverted FlatList displays activity indicator at the bottom

Open quadsurf opened this issue 7 years ago • 45 comments

Is this a bug report?

yes

Have you read the Contributing Guidelines?

yes

Environment

Environment: all newer environments Packages: any packages Target Platform: any platform Version: 0.53.0 and all other versions of RN that support FlatList with the inverted attribute

Steps to Reproduce

  1. return <FlatList {...props} inverted data={[...]} refreshing={this.state.isLoading} onRefresh={this.someListUpdatingFunction} /> (just add the 'inverted' attribute to any RN <FlatList/> to reproduce, then try pulling down to refresh, then try pulling up to refresh)

Expected Behavior

onRefresh should allow users to refresh using the most common way that users have been trained to refresh data over the years, which is to "pull down to refresh" and see an ActivityIndicator spinning above the FlatList, never below it... this should be the default even when inverted={true}

Actual Behavior

user has to pull UP to refresh (with Activity Indicator at the bottom) ... literal vs intuitive: when taken "literally", i can agree that this is expected behavior since it is after all "inverted" ... however, when taken "intuitively", it's not so expected, especially for users... I don't think app users have ever been trained to pull up to refresh data, except for maybe in the "OfferUp" chat app!

Most Applicable Use Case

  • a chat app, where users pull down to load more chat history that's appended above, similar to how it's done in iOS Messages app
  • UIs like terminals, event logs, chat, etc... where it's common to insert new content from the bottom and load old content when the user scrolls to the top or pulls down to refresh

Demo

https://snack.expo.io/S1UOyWgSM (to reproduce, just add the 'inverted' attribute to any RN <FlatList/> component using v0.53.0 or any version of RN that supports FlatList)

quadsurf avatar Jan 11 '18 23:01 quadsurf

Thanks for posting this! It looks like you may not be using the latest version of React Native, v0.53.0, released on January 2018. Can you make sure this issue can still be reproduced in the latest version?

I am going to close this, but please feel free to open a new issue if you are able to confirm that this is still a problem in v0.53.0 or newer.

How to ContributeWhat to Expect from Maintainers

react-native-bot avatar Feb 24 '18 03:02 react-native-bot

Looks like this may have been closed too eagerly -- the bot didn't see if v53 or newer was used in your issue description.

I'd still argue it's unclear to mention "newer versions" without being explicit, so I'll leave this closed. Please do re-open and make sure to include a specific version.

hramos avatar Feb 24 '18 03:02 hramos

Hi, I have reproduced the same problem on V53!

Draccan avatar Feb 26 '18 15:02 Draccan

This issue is straddling the line between bug report and feature request. I'll therefore mark it as a Good first issue for someone to tackle if they would like to get their feet wet with making contributions to React Native.

hramos avatar Mar 05 '18 18:03 hramos

@hramos thank you for keeping this alive! I may be able to tackle this in a few weeks, but rather than changing the current behavior, I would just provide other developers with the option to choose for themselves which behavior they prefer between [pull up to refresh] or [pull down to refresh], while in inverted mode of course.

quadsurf avatar Mar 05 '18 18:03 quadsurf

@hramos I would love to work on this as my first issue, but is there a guide for building the React-Native source for iOS? All I see are the steps for Android.

amloelxer avatar Mar 14 '18 18:03 amloelxer

I don't think we have a guide for that, but building from source for iOS may not be necessary if the fix only involves JavaScript.

Open RNTester/RNTester.xcodeproj in Xcode and select the RNTester target, and any of the iPhone simulators: simulator screen shot - iphone x - 2018-03-14 at 11 58 09

Then open the Flat List Example:

simulator screen shot - iphone x - 2018-03-14 at 11 58 09

Confirm everything works as expected.

You may need to follow the instructions for installing your fork of React Native: http://facebook.github.io/react-native/docs/android-building-from-source.html#1-installing-the-fork

Then you can proceed to add your changes to Libraries/Lists/FlatList.js (or SectionList or VirtualizedList, as needed) in your fork, and re-run the Flat List Example to verify.

Thanks for letting me know these instructions need to be expanded! Let me know how it goes and we can work on improving the docs.

hramos avatar Mar 14 '18 19:03 hramos

@hramos will do! Thanks for the info

amloelxer avatar Mar 14 '18 19:03 amloelxer

If this has not been fixed .I would like to start working on this. Can someone confirm?

himanshuchauhan avatar Apr 16 '18 14:04 himanshuchauhan

@himanshuchauhan feel free. I spent an hour or two looking at it, but it seemed to me I couldn't do it with just Javascript, and then work and life happened. Curious to your solution if you end up doing it!

amloelxer avatar Apr 16 '18 17:04 amloelxer

I have been trying to fix this . But I am having issues running the RNTester/RNTester.xcodeproj . I am getting this -

no script url provided make sure packager is running or you have embedded a js bundle

himanshuchauhan avatar Apr 18 '18 10:04 himanshuchauhan

If anybody else isn't, I'll be taking a look at this

briank621 avatar Apr 27 '18 20:04 briank621

I don't think this can be solved with a purely Javascript implementation. The inverted attribute causes a scale transform of -1 on the ScrollView (which is then undone by each cell Item inside to make the cell appear upright). Since the activity indicator would normally appear at the top, it now appears below.

The activity indicator is being generated by the RefreshControl component attached to the ScrollView. A fix that I have in mind is changing the location of the frame of the RefreshControl, as discussed here, but I haven't had success doing that. From there we would have to change how RefreshControl triggers a refresh (which normally occurs when scrollY: 0), so that it happens when scrollY matches the height of the page.

Somebody else can take another try at this. There might be a simpler approach, but the main issue is from the scale transform of -1.

briank621 avatar Apr 30 '18 02:04 briank621

Is it really a bug at all? Inverted could mean an inverted refresh either

endiny avatar May 06 '18 21:05 endiny

@briank621 I thought the exact the same thing. I didn't spend a ton of time looking at it, but remember at the time I didn't think it could be done solely in JS without doing some hackey things. I'll try to take a peak again and maybe build React-Native from source when I have time.

amloelxer avatar May 07 '18 13:05 amloelxer

Thanks for posting this! It looks like your issue may be missing some necessary information. Can you run react-native info and edit your issue to include these results under the Environment section?

Thank you for your contributions.

react-native-bot avatar May 15 '18 20:05 react-native-bot

@quadsurf @hramos I am now also making a chat interface, I need to do a chat history load, really need this refresh reversal function.

giantss avatar May 24 '18 03:05 giantss

@giantss feel free to send a PR. This issue was marked as a "Good first issue" and it's up to the community to implement.

hramos avatar May 24 '18 17:05 hramos

I was looking at the issue while I can not see a feature in it. Suggest closing.

tl;dr pull down is needed for loading old data in inverted list.

In my understanding, there are generally two actions that can operate on a list view, 1) loading more (older) data, and 2) refresh the potential (newer) data.

For a normal list view,

normal

the edge is on top, so a user has to pull down to refresh (2), and scroll up to load more (1). I think this is because the list of this sort is more common, users feels more natural and intuitive for "pull down to refresh".

When a list is inverted,

inverted

the edge is on the bottom, so not only the data is inverted, but also the mentioned two operations. And it is the most logical approach because if we cling to "pull down to refresh", then semantically there is no way to load older data (2).

Back to the issue description:

Most Applicable Use Case a chat app, where users pull down to load more chat history that's appended above, similar to how it's done in iOS Messages app --------a) UIs like terminals, event logs, chat, etc... where it's common to insert new content from the bottom and load old content when the user scrolls to the top or pulls down to refresh --------b)

a) "pull down to load more chat history" is the operation 1). In the RNTester, the FlatList works exactly this way; b) "...load old content when the user scrolls to the top or pulls down to refresh". I think it's better not to use up the two gestures (i.e., pull down and scroll up) for "load old content 1)". Because it occupies the gesture for "insert new content 2)". Moreover, If the "scroll up to refresh" does not feel right, I would suggest entailing an update interval for "insert new content from the bottom", which could be natural as it feels like tail -f.

@hramos @quadsurf

holmeshe avatar Jun 21 '18 01:06 holmeshe

I just ran into this, and would like to flush out my use case, and comparison.

First, on iOS, we have a FlatList that used an onScroll hack to load previous conversations with inverted={true}. Since iOS's behaviour allows bounce scroll, this worked, and we could trigger our load history button.

This was broken on android, and we opted to fix this using a refresh control. In doing this, and removing our hack, now both iOS and Android require a pull from the bottom to top to trigger the refresh. This seems counter-intuitive, and I've never heard of any chat app requiring a pull up to load history.

The big issue here is the expected flow of a chat app is that recent/latest messages appear at the bottom, which inverted does for us, but it also inverts the refresh direction, which is unexpected.

I think there is one of two solutions: either let RefreshView or FlatList control the direction of the pull to trigger the Refresh, or set a prop on FlatList that specifies the vertical alignment of the items (default to top, but with options for center and bottom).

mrozbarry avatar Jul 03 '18 15:07 mrozbarry

I was able to fix this by wrapping the FlatList in a ScrollView, kept the FlatList inverted, and put the refreshcontrol on ScrollView.

mrozbarry avatar Jul 03 '18 16:07 mrozbarry

I personally think the behavior of the RefreshControl being at the bottom makes sense. When you are in a Chat application, where new messages appear at the bottom and you keep the user scrolled at the bottom, it feels natural to pull from the bottom to up to try to refresh the conversation. Slack implements it like that. Having a RefreshControl at the top is impossible due to scrolling top causing to load more messages.

dozoisch avatar Oct 18 '18 18:10 dozoisch

@dozoisch IMO your statement makes sense if a user needs to refresh to see the latest messages. However, for most chat apps, new messages will always be updated automatically without having to refresh at all. Users only pull down in order to see history.

ChenLi0830 avatar Nov 28 '18 06:11 ChenLi0830

Inspired by @mrozbarry, here is what I did as a workaround.

const { fetchingMore, onFetchMore } = this.props;
return <ScrollView
  contentContainerStyle={{
    flexDirection: 'row',
    alignSelf: 'flex-end',
    flexGrow: 1
  }}
  refreshControl={<RefreshControl refreshing={fetchingMore} onRefresh={onFetchMore} />}
>
  <FlatList
    inverted
    style={styles.list}
    data={messages}
    keyExtractor={this.extractItemKey}
    renderItem={this.renderItem}
  />
</ScrollView>

Note that it's important to have the flexGrow: 1 style in scrollView, otherwise the FlatList would flow at the top of the scrollview when it's not full.

ChenLi0830 avatar Nov 28 '18 06:11 ChenLi0830

@ChenLi0830 It makes sense for most chat apps like what you said, However, if the flatList is for showing restaurant list or whatever similar things, then it doesn't make sense. In this case, RefreshControl should be in the bottom. The more fetching items, the more difficult to scrolling up to refresh in the situation user fetched lots of items already and looking at items in the bottom.

MingyuJeon avatar Dec 15 '18 06:12 MingyuJeon

Instruction for installing the forked version in this comment is broken. Could someone direct me where I can find it?

kiranjd avatar Jul 04 '19 14:07 kiranjd

users have been trained to refresh data over the years, which is to "pull down to refresh" and see an ActivityIndicator spinning above the FlatList, never below it.

In WhatsApp Inverted FlatList (image on the right) Pull Down displays the older chat history.

In WhatsApp Inverted FlatList (image on the right) Pull Up triggers the refresh:

I agree that the ActivityIndicator should show on top and never on the bottom and I will try to implement this solution, please let me know if I misunderstood the requirement. Thanks a lot.

fabOnReact avatar Apr 17 '20 15:04 fabOnReact

RefreshControl prop progressViewOffset works only on Android https://github.com/facebook/react-native/issues/10718.

progressViewOffset allows to change the position of the RefreshControl in a FlatList on Android.



export default function App() {
  const [refreshing, setRefreshing] = useState(false);  
  const screenHeight = Dimensions.get('window').height;
  const onRefresh = () => {
    setRefreshing(true)
  }

  return (
    <FlatList
      data={POSTS}
      renderItem={({ item }) => <Item data={item} />}
      keyExtractor={item => item.id}
      refreshControl={<RefreshControl progressViewOffset={screenHeight - 100} refreshing={refreshing} onRefresh={onRefresh} />}
      inverted
    />
  );
}

You can try the following snack on an Android device. The RefreshControl will show on top of the screen in an inverted FlatList and as explained in https://github.com/facebook/react-native/issues/10718 it does not work on iOS, I will work on another issue as I focus on Android.

iOS developer should read the pull requests and discussions related to https://github.com/facebook/react-native/issues/10718 to find a solution. Thanks a lot :smiley:

fabOnReact avatar Apr 21 '20 14:04 fabOnReact

I don't have any context on progressViewOffset but it is interesting that that exists.

If that existed on iOS does that feel like a more proper solution to this problem?

I'd like to avoid adding yet-another-prop to FlatList in https://github.com/facebook/react-native/pull/28857 if there is an existing prop that is intended to enable this behavior.

elicwhite avatar May 09 '20 01:05 elicwhite

If that existed on iOS does that feel like a more proper solution to this problem?

Yes, but it does not work on iOS with the changes to RCTScrollView from https://github.com/facebook/react-native/commit/1d22f8fb27e9432e357401ce6f6cede4d710472c, as Pull Request https://github.com/facebook/react-native/pull/11356 never made it to master.

iOS Developer should check the diff and comments from RCTRefreshControl 11356 and publish a new pull request.

Currently I am working on a pull request for https://github.com/facebook/react-native/issues/18266#issuecomment-624123006 and CircleCI, so I would avoid working with iOS. Thanks a lot :smiley: I wish you a good weekend :smiley:

fabOnReact avatar May 09 '20 09:05 fabOnReact