moxystudio / react-carousel

A React carousel component that aims to be as flexible as possible
MIT License
14 stars 3 forks source link

Fix child count & Click Behavior #26

Open paulobmarcos opened 4 years ago

paulobmarcos commented 4 years ago

Hi again! 😄

So after the fix, I found an error that was occurring due to the slide count.

The recent change filtered nullish children. To better reflect this change, we need to change componentDidUpdate to have a matching "count":

const previousCount = React.Children.toArray(prevProps.children).length;
const slideCount = React.Children.toArray(this.props.children).length;

if (previousCount !== slideCount) {
    this.setState({ slideCount });
}

I also had a problem with the "click" behavior. Until now, I think there was never a case for a slide to be a link or to do something on click.

Lets use this amazing website as an example 😁: image

Imagine that every slide/photo was a link to a page. What would happen if you clicked one of the slides that is not fully visible?

Right now, the Carousel starts animating to fully display the item and, at the same time, a click event will occur and the browser will forward to the url of that item.

This behavior seems strange because the user didn't see the item yet but is already being redirected to the correspondent page.

My suggestion would be to prevent the click event if an animation to fully reveal the item is happening/will happen.

One way to achieve this:

{ React.Children.toArray(children).map((child, i) => (
    React.cloneElement(child, {
        ...child.props,
        key: i,
        className: classnames('rc-slide', {
            '-current': current === i,
            [modifierCurrentClassName]: current === i,
        }, child.props.className),
        onClick: (ev) => this.handleSlideClick(ev, i, child),         // => Add an onClick handler to each child.
        onMouseUp: (ev) => this.handleSlideMouseUp(ev, i),
        onDragStart: (ev) => ev.preventDefault(),
        onDrag: (ev) => ev.preventDefault(),
    })
)) }
handleSlideClick = (ev, slideIndex, slide) => {
    if (this.animating) {
        ev.preventDefault();

        return;
    }

    if (slide.props.onClick) {
        slide.props.onClick(ev, slideIndex);
    }
};

What do you think? @jgradim @satazor


PS: I think I deserve a contributor prize to let me help you and be able to commit to a branch 👀😂

satazor commented 4 years ago

@paulobmarcos both sounds reasonable to me. I've added you as a contributor to this repository, so it's a bit easier for you to help maintaining this library.