Right place to update a state - React JS

Question

I am creating a simple slider component in React JS. I have three buttons Restart, Previous, Next. Clicking the previous will decrement the current state by 1, and will do continue until the current state equals 0. Also, the next button will increment the state until it equals slides.length-1. The Slider works fine for me, the issue is when the last slide approaches the 'Next' button is still enabled, but when I click it, it becomes disabled then. The expected behaviour is when the last slide is rendered the button should be disabled, and not an additional click should be required. The same issue is with the previous button.

You can check the behaviour here: https://codesandbox.io/s/react-live-sandbox-krfjk

I am sure, there's some issue with updating the state, I might be updating it in a wrong place, that's what not clicking my mind.

Answer 1

Problem in your code is that you are not updating the currentSlide before you check if previous or next button should be disabled.

When any button is button is pressed, first update the currentSlide then check which button should be enabled or disabled.

nextHandler function should look like this

nextHandler() {
    this.setState(
        prevState => ({ currentSlide: ++prevState.currentSlide}),
        () => {
          if (this.state.currentSlide < this.props.slides.length - 1) {
            this.setState({ prevDisabled: false });
          } else {
            this.setState({ nextDisabled: true });
          }
        }
    );
} 

and prevHandler should be like this

prevHandler() {
    this.setState(
      prevState => ({ currentSlide: --prevState.currentSlide}),
      () => {
        if (this.state.currentSlide === 0) {
          this.setState({ nextDisabled: false, prevDisabled: true });
        } 
      }
    );
}

Answer 2

Inside your nextHandler() change the setState to this:

this.setState({
      currentSlide: this.state.currentSlide + 1,
      nextDisabled: this.state.currentSlide + 1 === this.props.slides.length -  1,
      prevDisabled: false,
});

---

Considering the simplicity of your app, you do not need the prevState - You also don't need to define every state field in the return (react will implicitly fill those in as their last value):

// This can be just prevState => since you aren't using props
this.setState((prevState, props) => {
  if (prevState.currentSlide < this.props.slides.length - 1) {
    return {
      currentSlide: prevState.currentSlide + 1,
      prevDisabled: false, 
    };
  } else if (prevState.currentSlide === this.props.slides.length - 1) { // Issue is here
    return {
      nextDisabled: true,
      prevDisabled: false, // Don't need to keep defining this
    };
  }
});

The reason it didn't work is because you were using the following to check if the button should be disabled:

prevState.currentSlide === this.props.slides.length - 1

The prevState will be 1 count behind what you intend. You need to use the current state to do this. So another fix would be to change it to:

prevState.currentSlide + 1 === this.props.slides.length - 1

You can apply a similar fix to the prevHandler().

Answer 3

You have the same behavior when clicking "prev" button as well. It may be easier to derive the disabled state versus trying to compute it and store it in state.

Slides.js render function

render() {
  const { slides } = this.props;
  return (
    <div>
      <div id="navigation">
        <button
          data-testid="button-restart"
          onClick={this.resandler}
          data-testid="button-restart">
          Restart
        </button>
        <button
          data-testid="button-prev"
          onClick={this.prevHandler}
          data-testid="button-prev"
          // if current slide is 0, there is no previous
          disabled={this.state.currentSlide === 0}>
          Prev
        </button>
        <button
          data-testid="button-next"
          onClick={this.nextHandler}
          data-testid="button-next"
          // if current slide is slides.length - 1, there is no next
          disabled={this.state.currentSlide === this.props.slides.length - 1}>
          Next
        </button>
      </div>
      {slides.map((slide, i) => {
        return i === this.state.currentSlide ? (
          <div id="slide" key={slide + i}>
            <h1 data-testid="title">{slide.title}</h1>
            <p data-testid="text">{slide.text}</p>
          </div>
        ) : null;
      })}
    </div>
  );
}