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

Carousel has implicit and undocumented expectations on its children #1328

Open
kraenhansen opened this issue Dec 20, 2018 · 7 comments
Open

Comments

@kraenhansen
Copy link
Contributor

kraenhansen commented Dec 20, 2018

  • components: Carousel
  • reactstrap version 6.5.0
  • import method es
  • react version 16.6.3
  • bootstrap version 4.1.3

What is happening?

The API for the Carousel has implicit and undocumented expectations on its children:

  • It throws if mounted without children.
  • It cannot render with CarouselIndicators and individual CarouselItem elements.

I also don't understand why CarouselIndicators takes an array of items instead of a count (or does not just get this injected from the wrapping Carousel component).

What should be happening?

It should be possible to render a Carousel with CarouselIndicators and single CarouselItem followed by a list of CarouselItem components. Also it should be possible to render the Carousel without any children (useful if these are fetched from a remote source).

Steps to reproduce issue

  1. Render a Carousel without any children.

or

  1. Render a Carousel with a CarouselIndicators and single CarouselItem followed by an array of CarouselItem components.

Error message in console

TypeError: carouselItems.map is not a function
    at Carousel.renderItems (/.../node_modules/reactstrap/dist/reactstrap.cjs.js:2899:23)
    at Carousel.render (/.../node_modules/reactstrap/dist/reactstrap.cjs.js:2973:14)
    at finishClassComponent (/.../node_modules/react-dom/cjs/react-dom.development.js:14301:31)
    at updateClassComponent (/.../node_modules/react-dom/cjs/react-dom.development.js:14264:10)
    at beginWork (/.../node_modules/react-dom/cjs/react-dom.development.js:15082:16)
    at performUnitOfWork (/.../node_modules/react-dom/cjs/react-dom.development.js:17820:12)
    at workLoop (/.../node_modules/react-dom/cjs/react-dom.development.js:17860:24)
    at renderRoot (/.../node_modules/react-dom/cjs/react-dom.development.js:17946:7)
    at performWorkOnRoot (/.../node_modules/react-dom/cjs/react-dom.development.js:18837:7)
    at performWork (/.../node_modules/react-dom/cjs/react-dom.development.js:18749:7)
    at performSyncWork (/.../node_modules/react-dom/cjs/react-dom.development.js:18723:3)
    at requestWork (/.../node_modules/react-dom/cjs/react-dom.development.js:18592:5)
    at scheduleWork (/.../node_modules/react-dom/cjs/react-dom.development.js:18401:5)
    at Object.enqueueForceUpdate (/.../node_modules/react-dom/cjs/react-dom.development.js:12352:5)
    at WindowComponent.Component.forceUpdate (/.../node_modules/react/cjs/react.development.js:390:16)
    at CurrentWindow.getComponent.then.CurrentWindowComponent (http://localhost:8080/renderer.bundle.js:5526:18)
    at <anonymous>

Code

import React, { Component } from 'react';
import { Carousel, CarouselIndicators, CarouselItem } from 'reactstrap';

class Example extends Component {
  state = { activeIndex: 0 };

  render() {
    return (
      <Carousel
        activeIndex={this.activeIndex}
        next={this.next}
        previous={this.previous}
      >
        <CarouselIndicators
          items={[{}, {}, {}]} // 👈 Why is this not a number? Or even needed ...
          activeIndex={this.state.activeIndex}
          onClickHandler={this.goToIndex}
        />
        <CarouselItem>Slide 1</CarouselItem>
        <CarouselItem>Slide 2</CarouselItem>
        <CarouselItem>Slide 3</CarouselItem>
      </Carousel>
    )
  }

  next = () => {
    this.setState({ activeIndex: activeIndex + 1 });
  };

  previous = () => {
    this.setState({ activeIndex: activeIndex + 1 });
  };

  goToIndex = (index) => {
    this.setState({ activeIndex: index });
  };
}

export default Example;

https://stackblitz.com/edit/reactstrap-cmtxoq?file=Example.js

@xavierartot
Copy link

Your link is broke 👎

@kraenhansen
Copy link
Contributor Author

kraenhansen commented Jan 17, 2019

@xavierartot the Stackblitz? It works for me™.

It contains the same code as I posted above, so it's not strictly needed to reproduce the error.

@darkphoenixff4
Copy link

I see the same thing. In fact, Carousel throws an error in any situation in which it doesn't have direct CarouselItem children. Even if it's because there's a custom component between Carousel and CarouselItem that doesn't render anything.

@TheSharpieOne
Copy link
Member

IMHO Carousel should be rewritten/refactored. Several things need to be addressed; the easy of use of being the main one.

@Dakkers
Copy link
Contributor

Dakkers commented Apr 6, 2020

Something else that needs to be addressed: if I have a Carousel with 2 items, and I have wrap=true, then clicking "next" on the last item will actually go left instead of right because the code only looks at the change in activeIndex and it can't tell the direction simply from going 1 to 0. (that could be left, or right.)

@quickshiftin
Copy link

It also appears the error TypeError: carouselItems.map is not a function is raised if CarouselItem instances are not supplied as an array. I'd expect something like this to work (shorthand example):

<Carousel>
  <CarouselItem />
</Carousel>

@syamjayaraj
Copy link

syamjayaraj commented Apr 4, 2024

if your images data array is empty, just add an empty array like below.

const items =
modalData.images && modalData.images.length !== 0 ? modalData.images : [];

Refer the complete code:-

const [activeIndex, setActiveIndex] = useState(0);
  const [animating, setAnimating] = useState(false);
  const [modalData, setModalData] = useState({
    prices: [],
  });
  const [showMenu, setShowMenu] = useState(false);

  const items =
    modalData.images && modalData.images.length !== 0 ? modalData.images : [];
    
  let onExiting = () => {
    setAnimating(true);
  };

  let onExited = () => {
    setAnimating(false);
  };

  let next = () => {
    if (animating) return;
    const nextIndex = activeIndex === items.length - 1 ? 0 : activeIndex + 1;
    setActiveIndex(nextIndex);
  };

  let previous = () => {
    if (animating) return;
    const nextIndex = activeIndex === 0 ? items.length - 1 : activeIndex - 1;
    setActiveIndex(nextIndex);
  };

  let goToIndex = (newIndex) => {
    if (animating) return;
    setActiveIndex(newIndex);
  };

  const slides = items.map((item, index) => {
    return (
      <CarouselItem onExiting={onExiting} onExited={onExited} key={index + 123}>
        <img src={item} className={styles.modalImage} />
      </CarouselItem>
    );
  });

  let handleModalOpen = (item) => {
    setModalData(item);
  };

  let handleModalClose = () => {
    setModalData({
      prices: [],
    });
  };
  return(
            <div>
              <Carousel
                activeIndex={activeIndex}
                next={next}
                previous={previous}
              >
                <CarouselIndicators
                  items={items}
                  activeIndex={activeIndex}
                  onClickHandler={goToIndex}
                />
                {slides}
                <CarouselControl
                  direction="prev"
                  directionText="Previous"
                  onClickHandler={previous}
                />
                <CarouselControl
                  direction="next"
                  directionText="Next"
                  onClickHandler={next}
                />
              </Carousel>
            </div>)

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

No branches or pull requests

7 participants