oliviertassinari / react-swipeable-views

A React component for swipeable views. :snowflake:
https://react-swipeable-views.com/
MIT License
4.46k stars 480 forks source link

Strange behavior when combining HOCs #156

Closed leMaik closed 8 years ago

leMaik commented 8 years ago

For a component, I combine three HOCs and I noticed something strange: bindKeyboard(virtualize(autoPlay(SwipeableViews))) displays non-existant pages when using arrow keys to navigate while virtualize(autoPlay(bindKeyboard(SwipeableViews))) works as expected.

The HOCs shouldn't depend on the order they are applied, should they?

oliviertassinari commented 8 years ago

The HOCs shouldn't depend on the order they are applied, should they?

The HOC composition order matters. virtualize needs to be the last one called.

Edit: Hummm. I think you are right. Something weird is going on here.

leMaik commented 8 years ago

Indeed, the order does matter, but it seems strange to me.

oliviertassinari commented 8 years ago

I think I know what's going on. bindKeyboard as autoPlay are using the children property to determine the number of displayed slides. So he knows the bounds. Hence what you are describing is the expected result.

However, using virtualize with slideCount and bindKeyboard or autoPlay won't goes well when we reach the edge. That could easily be fixed by making the bindKeyboard and the autoPlay HOC looking at the slideCount property insted of Children.count(this.props).

oliviertassinari commented 8 years ago

To sum up.

  1. There is one bug with slideCount.
  2. The documentation is laking a section on the composition of HOCs.
oliviertassinari commented 8 years ago

@leMaik Turns out, the issue was more global than my previous points. I should have thought about the composition is the first place. It's going to be fixed with the next release.

leMaik commented 8 years ago

@oliviertassinari Glad that this behavior is at least documented now. :+1:

oliviertassinari commented 8 years ago

Notice that the right usage is autoPlay(bindKeyboard(virtualize(SwipeableViews))).

leMaik commented 8 years ago

The right usage doesn't work for me, though. :confused: I'll keep it "wrong" until the right usage works. :smile: After pressing the right arrow key, the slide is missing and the console is flooded with Warning: react-swipeable-view: the new index: NaN is out of bounds: [0-0].

oliviertassinari commented 8 years ago

Could you try out the v0.7.7 release? I think that it's fixed now.

leMaik commented 8 years ago

Oh, my bad. Updating to 0.7.7 fixed it.

nimahkh commented 6 years ago
<AutoPlaySwipeableViews
                containerStyle={{height: 'auto', width: '20%'}}
                onChangeIndex={this.changeIndex}
                enableMouseEvents>
                {items.map(item => (
                    <div className={[classes.slide].join(" ")} key={item.postId}>
                        <img draggable={false} src={item.coverImage} className={classes.image}/>
                        <Link to={`/tv/${item.postId}/${item.title.replace(/ /g, "-").replace(/\//g, "")}`}>
                            <Typography variant={'title'} className={classes.titleHead}>
                                {item.title}
                            </Typography>
                        </Link>
                    </div>
                ))
                }
            </AutoPlaySwipeableViews>
changeIndex = (index) => {
        const {items} = this.props;
        const remain = items.length % 5;

        if (!Number.isNaN(index)) {
            if (remain > 0) {
                if (!Number.isNaN(index)) {
                    this.setState({index: index + 5 > items.length - 1 ? (items.length - 1) - index : index + 4})
                }
            }
        }
    };

@oliviertassinari this is my code and swipable views is 13, but index is NAN and error is showing in console