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

indices mixing #183

Closed
6axter82 opened this issue Oct 12, 2017 · 5 comments
Closed

indices mixing #183

6axter82 opened this issue Oct 12, 2017 · 5 comments
Labels

Comments

@6axter82
Copy link

6axter82 commented Oct 12, 2017

I have encountered an issue. in the onSnapToItemI do the following:

const { app, project, } = this.props.navigation.state.params;
...
<Carousel
...
loop={true}
loopClonesPerSide={10}
onSnapToItem={(index) => {
    console.log('current index of the Card is ', index);
    if (app[index].project._id != project._id) {
        console.log('blah');
    }
    // later usage of app[index] is not possible
    // when scrolling fast into the left direction, this sets index to negative value
}}
/>

I have three Cards and do 10 clones per side in order to catch up on the gaps at the edges (my previous issue described in #12). I get the print outs of the indices, and when I scroll fast than I get negative index, which is wrong as I want to check on app[index].

this is a print out of Chrom console.log(...)

@screen shot 2017-10-12 at 14 40 23

screen shot 2017-10-12 at 14 41 31

I tried to play with loopClonesPerSide setting it to 3 and 5.

Setting it to 3 with carousel of 3 cards does not break the app, as the indices of newly added cards on the left side do not exceed 2,1,0;

In case of loopClonesPerSide={5} scrolling fast to the left edge before the new cards are added makes the index set to -2, and scrolling to the last card on the right side sets the index to 4. which is out of scope of the cards array.

screen shot 2017-10-12 at 15 08 09

As a work around maybe implement a new property for Carousel, which is move to next card at swipe and not further. Which is not a better solution as we want to scroll fast enough as we want and not go to the lets say 30's card one by one.

@bd-arc
Copy link
Contributor

bd-arc commented Oct 13, 2017

@6axter82 Regarding your issue, are you able to reproduce it in a production environment? As stated in this very important note, you unfortunately can't trust carousel's behavior in a debug environment on Android.

Note that there already is a prop that allows something along the lines of "slide by slide" scrolling: lockScrollWhileSnapping. It is kind of a hack because, ultimately, we've got no granular control over the underlying ScrollView.

Regarding the "jump" you're experiencing, it is linked to the fact that the scroll position is, indeed, repositioned when a clone or an edge has been reached. On iOS it works flawlessly, but on Android there sometimes is an ugly flicker. So far, I haven't been able to get to the root of the issue, even though I know that, again, it has to do with ScrollView's implementation. But Android is so random and buggy that this is a real nightmare to understand...

@bd-arc
Copy link
Contributor

bd-arc commented Oct 13, 2017

Well, forget about the app environment. I can see where the issue is coming from:

    _getDataIndex (index) {
        const { data, loopClonesPerSide } = this.props;
        const dataLength = data && data.length;

        if (!this._enableLoop() || !dataLength) {
            return index;
        }

        if (index >= dataLength + loopClonesPerSide) {
            return index - dataLength - loopClonesPerSide;
        } else if (index < loopClonesPerSide) {
            return index + dataLength - loopClonesPerSide;
        } else {
            return index - loopClonesPerSide;
        }
    }

Clearly, this will return negative values if loopClonesPerSide is bigger than data's length. I'm going to work on a fix right away!

@bd-arc
Copy link
Contributor

bd-arc commented Oct 16, 2017

@6axter82 This wasn't that easy, but this commit should fix the issue.

Can you try it and confirm that it works for you?

@6axter82
Copy link
Author

Hey, I tried, it works nice!

@bd-arc
Copy link
Contributor

bd-arc commented Oct 16, 2017

Good to know. The fix has been published in version 3.3.4 ;-)

@bd-arc bd-arc closed this as completed Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants