score:0

Accepted answer

Problem

state.user is used to set the initial value of your component's state. Changes to those props do not change your state after the component is created. They do change the values in your inputs because the initial value was an empty string '' so you default to showing the value from props. This is very misleading since those inputs don't reflect the current state.

I bet I could delete at least half of this code but that's besides the point. But take a moment to think about why props.propState === '' ? '' : props.propState is always exactly the same as just props.propState.


Solution

I have two key recommendations for how I would rewrite this:

  1. Select user by id
  2. Separate into multiple components
  3. Store only the modifications in the state

Create a selector function selectUserById that selects a user from your Redux state by the id. I don't think it makes sense to store the current user properties as top-level properties of state.user like you have them right now. It seems like you also have a property state.user.users which is an array of all loaded users so I would use that.

const selectUserById = (state, id) => state.user.users.find(user => user.id === id);

Or better yet, store an object of users keyed by id.

const selectUserById = (state, id) => state.user.users[id];

With this approach we either have a complete user object or we have undefined. It's easy to check for undefined and not show the "Update User" form at all until we have real data. This makes more sense than using empty strings as the default.


We can access the complete user object from Redux. I would not duplicate that object in state. Instead I would use the state only for the properties that you have changed. You would start out with the state as an empty object and add properties to it as you modify their inputs. You can always combine the two together using object spreading.

const merged = {...existing, ...changes}

Can you implement these suggestions using class components and connect? Yes. But why add the extra hurdle? Some of the things in your code like this.handleChange.bind(this) are relics of the past when we had to do that because there wasn't a better way. But now we have better ways so you should use them.


Code

Interactive Demo on CodeSandbox

import "./App.css";
import React, { useEffect, useState } from "react";
import { useSelector, useDispatch } from "../store";
import { getSingleUser, updateUser } from "../store/slice";

const selectUserById = (state, id) => state.user.users[id];

const UserIdForm = ({ submitId }) => {
  const [id, setId] = useState("");

  const handleSubmit = (event) => {
    event.preventDefault();
    submitId(id);
  };

  return (
    <form onSubmit={handleSubmit}>
      <div>
        <label>ID:</label>
        <input
          type="text"
          value={id}
          onChange={(event) => setId(event.target.value)}
        />
      </div>
      <div>
        <input type="submit" value="Submit" />
      </div>
    </form>
  );
};

const UpdateUserForm = ({ id }) => {
  const [changes, setChanges] = useState > {};

  const existing = useSelector((state) => selectUserById(state, id));

  const dispatch = useDispatch();

  // respond to changes in id by clearing the changes state and requesting the user
  useEffect(() => {
    dispatch(getSingleUser(id));
    setChanges({});
  }, [dispatch, setChanges, id]);

  if (!existing) {
    return <div>Loading User...</div>;
  }

  const merged = { ...existing, ...changes };

  const handleChange = (property, event) => {
    // in function components you have to copy the whole state
    setChanges((prevChanges) => ({
      ...prevChanges,
      [property]: event.target.value
    }));
  };

  const handleUpdate = (event) => {
    event.preventDefault();
    const postData = { ...merged, id };
    console.log("POSTDATA:", postData);
    dispatch(updateUser(postData));
  };

  const renderInput = (property, label) => {
    return (
      <div>
        <label>
          {label}
          <input
            type="text"
            value={merged[property]} // shows the current value or the updated value
            onChange={(event) => handleChange(property, event)}
          />
        </label>
      </div>
    );
  };

  return (
    <form onSubmit={handleUpdate}>
      {renderInput("first_name", "First Name:")}
      {renderInput("last_name", "Last Name:")}
      {renderInput("phone", "Phone:")}
      {renderInput("email", "Email:")}
      <div>
        <input type="submit" value="Submit" />
      </div>
    </form>
  );
};

const UsersContainerUpdate = () => {
  // this is the id that was last submitted.
  const [id, setId] = useState();

  return (
    <div>
      <div>
        <h1>Update User By ID</h1>
        <UserIdForm submitId={setId} />
      </div>
      {!!id && ( // only load when there is an actual id
        <div>
          <h1>Update User</h1>
          <UpdateUserForm id={id} />
        </div>
      )}
    </div>
  );
};

export default UsersContainerUpdate;

score:2

If your requirement is just to state after API call inside this.props.getSingleUserData(id),

Approach 1: (Unclean) Add one more argument to getSingleUserData(id, setState) and pass it this.setState as an argument and inside getSingleUserData you can set the state using the function reference passed

Approach 2: You can return a promise from getSingleUserData and do setState once it is resolves

Suggestion: Divide your big component into individual components (like one for getting user ID and one for User data updation). The more we identify and split our project into meanigfull individual components we get more clean codes. Also when you choose to move towards functional components you can reduce lot of boiler plates with hooks.


Related Query

More Query from same tag