fix: item separator issue

[Buksirchik]

Description

Fixes (#633)

Issue summary:
The ItemSeparatorComponent doesn't render properly in a case when adding items to the bottom (e.g. infinity scroll). It causes because a separator is rendered at the bottom of the cell and when new data is added, previous items don't render due to memoization.

Reviewers’ hat-rack 🎩

Due to the placement of ItemSeparatorComponent has been changed from bottom to top, we have to change the leading and trailing items.
Instead of these statements:

const leadingItem = this.props.data[index];
const trailingItem = this.props.data[index + 1];

we should use:

const leadingItem = this.props.data[index - 1];
const trailingItem = this.props.data[index];

Screenshots or videos (if needed)

Horizontal inverted FlashList (android, iOS)

Screen.Recording.2023-05-14.at.13.53.32.mov

Screen.Recording.2023-05-14.at.14.22.16.mov

Horizontal FlashList (android, iOS)

Screen.Recording.2023-05-14.at.13.54.08.mov

Screen.Recording.2023-05-14.at.14.21.25.mov

Vertical FlashList (android, iOS)

Screen.Recording.2023-05-14.at.13.55.09.mov

Screen.Recording.2023-05-14.at.14.24.02.mov

Vertical inverted FlashList (android, iOS)

Screen.Recording.2023-05-14.at.13.57.40.mov

Screen.Recording.2023-05-14.at.14.24.39.mov

Checklist

• I have added a changelog entry following the Keep a Changelog guidelines.

[Buksirchik]

I have signed the CLA!

[Buksirchik]

Hi @naqvitalha
Could you please review that PR?

[chj-damon]

I'm facing the same problem, can this PR be merged soon? @naqvitalha

[chj-damon]

this PR has been two months now, can we make any progress here? @naqvitalha

[Gondolav]

Hi, I'm also facing the same problem @fortmarek @naqvitalha

[irion94]

I've created a patch from your PR and encountered an issue when the list works with Sticky Headers.

Upon further investigation, I believe a slightly different approach may be beneficial. We can make trailingItem undefined, allowing developers more flexibility to decide what should be rendered in this situation.

This change provides greater control and customization options when working with the list. Your feedback on this approach is appreciated.

Thank you for raising this issue. I can confirm its existence, but I believe it's crucial for us to take some time to carefully consider the best approach for addressing it. ✌️

  private separator = (index: number) => {
    // Make sure we have data and don't read out of bounds
    if (this.props.data === null || this.props.data === undefined) {
      return null;
    }

    const leadingItem = this.props.data[index];
    const trailingItem = this.props.data[index + 1];

    const props = {
      leadingItem,
      trailingItem,
      // TODO: Missing sections as we don't have this feature implemented yet. Implement section, leadingSection and trailingSection.
      // https://github.com/facebook/react-native/blob/8bd3edec88148d0ab1f225d2119435681fbbba33/Libraries/Lists/VirtualizedSectionList.js#L285-L294
    };
    const Separator = this.props.ItemSeparatorComponent;
    return Separator && <Separator {...props} />;
  };

[Buksirchik]

@irion94 Hi, I appreciate your feedback about my PR. But I don't understand your problem. Could you elaborate problem more clearly?
As I understand, the ItemSeparatorComponent isn't rendered for a sticky header. Is that right?

[irion94]

@Buksirchik, I apologize for not providing more details earlier. To illustrate the issue, I've prepared a basic example that showcases the behavior on the release version of the flashlist and with your patch applied.
It's not about the ItemSeparatorComponent isn't rendered. Rather, the concern lies in the behavior of the animation between section headers.

In addition, I've included a snippet of my Separator component (although I didn't anticipate it causing the problem).
As we examine the behavior, it appears that the section headers are being calculated incorrectly.

I'm not certain if /fixture includes an example of section header usage. Therefore, it might be a good idea to review the behavior of your fix independently

flashlist.main.mov

bug.item-separator.mov

export const __Separator = ({
	leadingItem,
	trailingItem,
}: SeparatorProps) => {
	if (trailingItem.type === 'type-a' && leadingItem.type === 'type-b')
		return <Separator size={'BIG'} />;

	if (trailingItem.type === 'type-b' && leadingItem.type === 'type-b')
		return <Separator size={'SMALL'} />;

	if (trailingItem.type === 'type-c' && leadingItem.type === 'type-b')
		return <Separator size={'SMALL'} />;

	if (trailingItem.type === 'type-b' && leadingItem.type === 'type-c')
		return <Separator size={'SMALL'} />;
	if (trailingItem.type === 'type-a' && leadingItem.type === 'type-d')
		return <Separator size={'BIG'} />;

	return null;
};

[Buksirchik]

Upon further investigation, I believe a slightly different approach may be beneficial. We can make trailingItem undefined, allowing developers more flexibility to decide what should be rendered in this situation.

Also I don't understand why do you need it?

The main point is that ItemSeparatorComponent should be shown if we have quantity of items bigger than 1. If we have only 1 item, we shouldn't render ItemSeparatorComponent. My opinion on that — we shouldn't pass undefined to trailingItem.
On the other hand you can be right, but it's out of scope of that issue. 😊

[Buksirchik]

@irion94, according to your issue, I'll try to invistigate soon. Thanks for passing more details. Also I want to know, do you have any custom animation in your example? Did you only use stickyHeaderIndices?

[irion94]

@irion94, according to your issue, I'll try to invistigate soon. Thanks for passing more details. Also I want to know, do you have any custom animation in your example? Did you only use stickyHeaderIndices?

I'm using stickyHeaderIndices and nothing else. I tried to test your code on the most basic example 😀

Upon further investigation, I believe a slightly different approach may be beneficial. We can make trailingItem undefined, allowing developers more flexibility to decide what should be rendered in this situation.

Also I don't understand why do you need it?

The main point is that ItemSeparatorComponent should be shown if we have quantity of items bigger than 1. If we have only 1 item, we shouldn't render ItemSeparatorComponent. My opinion on that — we shouldn't pass undefined to trailingItem. On the other hand you can be right, but it's out of scope of that issue. 😊

Yes, I agree, It would be better to handle it as it is right now, but if we don't find a way out I'll keep my version 😅

Anyway, please confirm if the problem truly occurs and let me know ✌️

[Buksirchik]

Anyway, please confirm if the problem truly occurs and let me know ✌️

@irion94 I confirm the issue. I reproduced it.

[Buksirchik]

Guys, I've decided to close that PR due to fix isn't good enough. It causes a lot of problem with animations and sticky headers (see problem described by @irion94). It seems the better way to handle separator via renderItem.

PS. I'm sorry about long feedback, it took about 1.5 year :(