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

Adjust index if data item is removed #157

Open
Gerharddc opened this issue Sep 21, 2017 · 17 comments
Open

Adjust index if data item is removed #157

Gerharddc opened this issue Sep 21, 2017 · 17 comments

Comments

@Gerharddc
Copy link

I want to use this component to let a user navigate through a (potentially endless) stream of items and as such I am adding new items to the data array as the user nears the end of the list and this works just perfectly. Obviously though I can't just keep adding items without removing older ones forever as this will exhaust memory at some point.

Removing items from the start of the data array seems to work pretty well except for the fact that it seems to act very strange with the index system. The problem is that the index doesn't change and after navigation there is a huge jump to the position of the old index+1. It would be nice if the component could automatically deal with this or provide me with a way to decrement the index without visually navigating.

@bd-arc
Copy link
Contributor

bd-arc commented Sep 22, 2017

Hi @Gerharddc,

I totally get what you're after, but this might be tricky to implement.

Would you mind providing me with a simple example that would serve as a starting point to work on this feature? Something based on the provided example maybe...

@Gerharddc
Copy link
Author

@bd-arc I'll have to see about getting it integrated into the example but I can already give you the relevant code that I tested with yesterday:

let idx = 0;

export default class Test extends React.Component {
	private getViewport() {
		return {
			width: Dimensions.get('window').width,
			height: Dimensions.get('window').height,
		};
	}

	state = {
		viewport: this.getViewport(),
		lys: [
			{ name: 'One' },
			{ name: 'Two' },
			{ name: 'Three' },
			{ name: 'Four' },
		],
	};

	private updateDimensions() {
		this.setState({
			viewport: this.getViewport(),
		});
	}

	private renderItem({ item, index }: { item: IItem, index: number }) {
		return (
			<View>
				<Image source={require('../../assets/img/cat.jpg')}
					style={{width: this.state.viewport.width - 100, height: this.state.viewport.width - 100}}/>
				<Text>{item.name}</Text>
			</View>
		);
	}

	private handleIndexChange(index: number) {
		const lys = this.state.lys.slice();

		if (index >= (lys.length - 2)) {
			idx++;
			lys.push({ name: idx.toString() });
		}

		if (index >= 5) {
			lys.splice(0, 1);
		}

		this.setState({ lys });
	}

	public render() {
		return (
			<View style={{ flex: 1 }}
				<Text>Flow</Text>
				<Carousel
					data={this.state.lys}
					renderItem={this.renderItem.bind(this)}
					sliderWidth={this.state.viewport.width}
					itemWidth={this.state.viewport.width - 100}
					onSnapToItem={this.handleIndexChange.bind(this)}
				/>
			</View>
		);
	}
}

The slightly strange syntax is just TypeScript.

This code adds a new item just before you reach the end of the carousel and also removes an item for each one that it adds. If you run this and try to scroll to the next item after 1 has been 1 then it will suddenly jump another item ahead because the index of the item that you were looking at before the interaction has actually moved 1 down. If you were to remove more than 1 item at a time this effects just becomes worse.

@bd-arc
Copy link
Contributor

bd-arc commented Oct 16, 2017

Hi @Gerharddc,

I've gave your issue some thought, but I don't see an easy way to deal with it (particularly with the recent loop addition which just made index handling even trickier). There is no definitive way of knowing the index that should be returned by a particular item after various additions/removals.

Do you have any suggestion about how this problem might be tackled?

@Gerharddc
Copy link
Author

Gerharddc commented Oct 16, 2017

@bd-arc I can definitely notify the carousel of exactly how many items I will be removing before, after or during the change. In this way it should be able to adjust adjust the internal index accordingly. Unfortunately I am unfamiliar with the code-base and do not know if this is something that can easily be implemented.

In theory I could get away with just a method to notify the carousel to adjust the index backwards by a certain amount.

@bd-arc
Copy link
Contributor

bd-arc commented Oct 16, 2017

@Gerharddc Thanks for the feedback! I'll see if I can think of something that doesn't break the existing logic.

FYI, index handling already consists of more than 100 lines of code...

@m-vdb
Copy link

m-vdb commented Nov 29, 2017

any progress on this? It looks like I ran into the same issue: when adding element to the data array, the carousel is buggy

@m-vdb
Copy link

m-vdb commented Nov 29, 2017

I've been able to work around my issue by using this._carousel.snapToItem(index + 1, false), maybe it can help you!

@bd-arc
Copy link
Contributor

bd-arc commented Nov 29, 2017

Hi @m-vdb,

I didn't have time to work on this issue lately, but if the problem is spreading I'll try to take a look at it shortly. Not before next week though, since I currently have major professional deadlines to meet ;-)

@raduflp
Copy link

raduflp commented Dec 7, 2017

having the same issue, when deleting the last item a blank slide was displayed until you started scrolling again. Solved it by adopting @m-vdb solution, but it feels hacky.
Still, great job and thanks for this lib.

componentDidUpdate(prevProps, prevState)  {
   if (this.props.data.length < prevProps.data.length) { 
     this._carousel.snapToItem(this.props.passengers.length, false);
  }
}

@Xaber20110202
Copy link

If use same data, or when the data length is 1 and we add data items, there will be buggy too, inspired by @raduflp , I use following hack code to treat these situations

// for same data buggy situation, use only length change or data change time
if (!isEqual(oriData, data)) {
    // for length is 1 situation
    this.setState({ data: data.length === 1 ? data.concat(data) : data }, () => {
        this._carousel.snapToItem(data.length, false)
    })
}

@bd-arc
Copy link
Contributor

bd-arc commented Jan 3, 2018

Hey guys,

I finally had time to dig deeper into the issue. I've tried different things, but I don't see any other solution than the one you've all came up with: to snap without animation to the relevant item when the data set is updated.

With FlatList's specific rendering mechanism, there is no way of keeping track of each item index when adding/removing them; that part has to be handled externally.

Still, if you have another idea of dealing with it, do not hesitate to share it! I'll gladly try to implement it ;-)

@bd-arc bd-arc added * and removed enhancement labels Jan 3, 2018
@bd-arc bd-arc closed this as completed Jan 3, 2018
@Gerharddc
Copy link
Author

Not sure why I did not think of this earlier, but I think we just need to pass a unique key with each item in the list. When the carousel receives new props, it can simply check which keys have disappeared in order to adjust its index automatically.

Please correct me if I am wrong.

@bd-arc
Copy link
Contributor

bd-arc commented Jan 18, 2018

@Gerharddc Well, that's an idea worth pursuing. I'll see if I can implement something along those lines.

If anyone wants to jump in and submit a PR, do not hesitate ;-)

@bd-arc bd-arc reopened this Jan 18, 2018
@ShaMan123
Copy link

Ran into this problem as well.
I gave it some thought and I came up with this:

        const predicate = '??';
        const prevIndex = prevProps.data.findIndex((value, index) => value === predicate);
        const nextIndex = this.props.data.findIndex((value, index) => value === predicate);
        const delta = nextIndex - prevIndex;
        this._activeItem += delta;

I think the right place in the code would be here.
I tested a simple static implementation (adding one child) => it worked:

             if (this._previousItemsLength !== itemsLength) {
                nextActiveItem = this._previousItemsLength < itemsLength ? nextActiveItem + 1 : nextActiveItem;
                this._hackActiveSlideAnimation(nextActiveItem, null, true, false);
            }

However this is far from a viable solution.

Implementing keys as suggested by @Gerharddc would solve the predicate value. and would make life easier for everyone, so it sounds.

I tried testing a better implementation of this but ran into a VERY strange bug. When state/props update after mount prevProps === this.props. I have no clue why this is happening. And have no idea how to work around it.

While testing I ran into a bug in the code (I think), that overrides the _hackActiveSlideAnimation method.
I'll submit a PR.

@dohooo

This comment was marked as spam.

@7dp
Copy link

7dp commented Dec 20, 2021

experiencing the same issue, any update about this?

@7dp
Copy link

7dp commented Dec 22, 2021

sorry, but did you find any solution about this? @Gerharddc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants