trussworks / react-uswds

USWDS 3.0 components built in React
https://trussworks.github.io/react-uswds/
Apache License 2.0
190 stars 81 forks source link

[fix] DatePicker component value not properly updated when modified by other component #2348

Open adil-ahorger opened 1 year ago

adil-ahorger commented 1 year ago

ReactUSWDS Version & USWDS Version:

"@trussworks/react-uswds": "4.2.0", "@uswds/uswds": "3.1.0",

Describe the bug

When the DatePicker value prop is set to some state value, and that state is updated by some other component, e.g. an input, the new value is not reflected in the DatePicker component. I tried assigning both the defaultValue and value props, but got the same results. I've also confirmed in the React Devtools that the prop value(s) are correctly updated, but the internal DatePicker state is not updated. Am I perhaps overlooking something?

I've pasted a minimal reproduction below.

const Test = () => {
  const [data, setData] = useState({
    statusId: 0,
    endDate: null,
  });

  return (
    <div className="grid-row">
      <div className="grid-col">
        <label htmlFor="end-date">
          End Date
        </label>
        <DatePicker
          id="end-date"
          name="end-date"
          minDate={new Date().toISOString().split('T')[0]}
          defaultValue={data.endDate}
          value={data.endDate}
        />
      </div>
      <div className="grid-col">
        <input
          id="active"
          type="radio"
          value={0}
          checked={data.statusId === 0}
          onChange={(e) =>
            setData((prevState) => ({
              ...prevState,
              endDate: null,
              statusId: +e.target.value,
            }))
          }
        />
        <label htmlFor="active">Active</label>
      </div>
      <div className="grid-col">
        <input
          id="disabled"
          type="radio"
          value={1}
          checked={data.statusId === 1}
          onChange={(e) =>
            setData((prevState) => ({
              ...prevState,
              endDate: new Date().toISOString().split('T')[0],
              statusId: +e.target.value,
            }))
          }
        />
        <label htmlFor="disabled">Disabled</label>
      </div>
    </div>
  );
};

Expected behavior Updating the value prop from somewhere else should properly update the DatePicker value.

haworku commented 1 year ago

👋 I think the one layer missing here is that you won't be able to imperatively set the value on an uncontrolled component (all react-uswds lib was originally written as uncontrolled components, I made an #2448 to describe that rationale more and where things have moved since then). Since DatePicker is uncontrolled, the only way the value changes after initial load is via user interaction, in this case interacting with the calendar picker or the user typing into the input field.

We have since started adding more functionality for consumers to take control over the components on their own by accepting PRs that wrap components in forwardRef (examples are included in the linked issue). Another way folks are handling this is via form libraries wrapped around our components. Third party libraries such as formik or react-hook-form which create their own version of the form state and take over control.

adil-ahorger commented 1 year ago

I appreciate the explanation of the library's rationale. Since raising this issue I actually did something similar to your suggestion. I created a few custom components for a project that wraps the react-uswds components (the Modal component, for instance) in order to implement some custom controlled logic.

Since my use case appears to be different from this library's intended design, I'm closing this issue.

sawyerh commented 1 year ago

Another way folks are handling this is via form libraries wrapped around our components. Third party libraries such as formik or react-hook-form which create their own version of the form state and take over control.

I think an example would be helpful if y'all have one. I ran into this same bug on a project using Formik and am scratching my head around the expected way to update the DatePicker value. The bug is in a part of the codebase originally written by Truss, fwiw, so I think even internally there may not be a solid understanding of this.

haworku commented 1 year ago

@sawyerh Oh no. Gotcha, thanks for commenting here. We need to look again then 🔍