1

I have a simple form, where I would like to remove the selected item on remove button click. I'm partially removing the desired item, but if I remove the middle item, it also removes the last item. If I remove the first item, it removes the entire array.

Live example here

code here:

import React from "react";
import ReactDOM from "react-dom";

import "./styles.css";

class App extends React.Component {
  constructor(props) {
    super(props);

    this.state = {
      data: ["Saab", "Volvo", "BMW"],
      editMode: false,
      rulesTerm: ""
    };
  }

    handleInput = e => {
    let value = e.target.value;
    let name = e.target.name;
    console.log(name);
    console.log(value);

    this.setState(prevState => ({
      data: {
        ...prevState.data,
        [name]: value
      }
    }));
  };

  handleFormSubmit = e => {
    e.preventDefault();

    const { data } = this.state;
    console.log(data);
  };

  removeRule = (e, index) => {
    e.preventDefault();

    const { data } = this.state;
    console.log("removed", data.splice(index));

    this.setState({
      data: data,
      rulesTerm: ""
    });
  };

  render() {
     return (
       <div className="App">
        <div>
          {!this.state.editMode ? (
            <button onClick={() => this.setState({ editMode: true })}>
              edit
            </button>
          ) : (
            <div>
              <button onClick={() => this.setState({ editMode: false })}>
                cancel
               </button>
              <button onClick={this.handleFormSubmit}>submit</button>
            </div>
           )}
        </div>

        <React.Fragment>
          {this.state.data.map((rule, index) =>
            this.state.editMode ? (
              <form onSubmit={this.handleFormSubmit}>
                <React.Fragment>
                  <input
                    onChange={this.handleInput}
                    type="text"
                    placeholder="Cars"
                    name={rule}
                    defaultValue={rule}
                    key={index}
                  />
                  <button onClick={event => this.removeRule(event, index)}>
                    Remover
                  </button>
                </React.Fragment>
              </form>
            ) : (
              <p key={rule}> - {rule} </p>
            )
          )}
        </React.Fragment>
      </div>
    );
  }
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

Any help? Thank you!

4
  • How about splice(index, 1) ?
    – Tmb
    Mar 1, 2019 at 15:21
  • Like that, If I remove the first 'Saab', it removes the wrong item. Weird
    – RCohen
    Mar 1, 2019 at 15:24
  • No it works, then the problem is on your input. If you change your "defaultValue" attribute to "value", you will see that the input value is good after delete, but you will not be able to update it anymore. You can check the React doc about the Form controlled component for more explanations : reactjs.org/docs/forms.html#controlled-components
    – Tmb
    Mar 1, 2019 at 15:38
  • yeah, you are right, it works with the value prop. Thank you :)
    – RCohen
    Mar 1, 2019 at 15:41

3 Answers 3

5

When using splice, you need to pass in the number of elements to delete.

If deleteCount is omitted, or if its value is equal to or larger than array.length - start (that is, if it is equal to or greater than the number of elements left in the array, starting at start), then all of the elements from start through the end of the array will be deleted.

2
  • If I add data.splice(1, index), it doesn't remove the first item
    – RCohen
    Mar 1, 2019 at 15:21
  • 1
    correct syntax is data.splice(index, 1) -- array.splice(start[, deleteCount[, item1[, item2[, ...]]]])
    – klhr
    Mar 1, 2019 at 15:27
0

One solution would be to convert your deleting function into a curried function, which will first receive the index when called once, and then the event.

You can filter out your element using the callback version of setState and filtering out the element with the same index :

removeRule = index => event => {
    this.setState(prev => ({
        data: prev.data.filter((el, i) => i !== index),
        rulesTerm: ""
    }));
};

You could also indicate your splice method that it only requires to remove a single element :

removeRule = index => event => {
    this.setState(prev => ({
        data: prev.data.splice(index, 1),
        rulesTerm: ""
    }));
};

Your function binding in the JSX will now look like the following :

<button onClick={this.removeRule(index)}>
    Remover
</button>
4
  • I'm not downvoting, but this adds unnecessary complexity while providing no advantage of what they are currently doing (eg. a new function is still defined on every render). Furthermore, while you mention it, your answer doesn't really focus on the real problem: improper use of splice. Mar 1, 2019 at 15:26
  • It seems like you did not read the solution at all. The code given in the new function shortens it and the second solution shows exactly how to use the splice function. The JSX is also way more readable without the arrow function being written inside of it. As opposed to the upvoted solution which is nothing but a copy/paste of the doc without any example. Using the callback version of setState is also the recommended way of doing anything if you plan on using old state values, just like in this case.
    – Treycos
    Mar 1, 2019 at 15:29
  • I mean, I read your answer - I'm just saying your answer first (and mostly) describes the advanced concept of currying and different ways of setting state, which requires refactoring the component. You mention how to use splice, but you didn't mention that as the real solution to the problem. While I agree your answer provides a cleaner component, that's not the point of the question and distracts from the problem at hand: the misuse of splice. Mar 1, 2019 at 15:37
  • Well, the wrong usage of splice was already mentioned in other solutions/comments so I did not think repeating it would be relevant, the only lacking thing being an example of it working. A single function is being changed, far from a real refactor, and the JSX is almost untouched. The OP also used the callback setState earlier, so I did not feel the need to explain how it works, the call is basically the same as the previous one with minor changes. I don't consider 5 lines to be a refactor.
    – Treycos
    Mar 1, 2019 at 15:42
0

In your case I would use a filter, and pass the rule as an argument:

 <React.Fragment>
          {this.state.data.map((rule, index) =>
            this.state.editMode ? (
              <form onSubmit={this.handleFormSubmit}>
                <React.Fragment>
                  <input
                    onChange={this.handleInput}
                    type="text"
                    placeholder="Cars"
                    name={rule}
                    defaultValue={rule}
                    key={index}
                  />
                  <button onClick={event => this.removeRule(event, rule)}>
                    Remover
                  </button>
                </React.Fragment>
              </form>
            ) : (
              <p key={rule}> - {rule} </p>
            )
          )}
        </React.Fragment>

Than in your removeRule function:

removeRule = (e, rule) => {
    e.preventDefault();

    const { data } = this.state;
    const newData = data.filter(item => item !== rule);
    console.log("removed");

    this.setState({
      data: newData,
      rulesTerm: ""
    });
  };
4
  • I'm going to try :)
    – RCohen
    Mar 1, 2019 at 15:25
  • If i choose to remove the middle item, it removes the last one :/
    – RCohen
    Mar 1, 2019 at 15:37
  • Do you have multiple items with the same name?
    – Dmitriy
    Mar 1, 2019 at 15:43
  • Hmm, the filter method should only remove the item with the name you're passing to it
    – Dmitriy
    Mar 1, 2019 at 15:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.