I have data in this form:
books = [{
id: 1,
name: "book-1",
audiences: [
{
audienceId: 1,
name: "Cat A",
critics: [
{ id: 5, name: "Jack" },
{ id: 45, name: "Mike" },
{ id: 32, name: "Amanda" }
],
readers: [
{ id: 11, fullName: "Mike Ross" },
{ id: 76, fullName: "Natalie J" },
{ id: 34, fullName: "Harvey Spectre" }
]
}]
The forms are nested. For each book, there are critics and readers, and would render each form depending on audienceId value
<div className="App">
{books.map((book, index) => (
<div key={book.id}>
<h1>Books {index + 1}</h1>
{book.audiences.map((au) => (
<div key={au.audienceId}>
<h3>{au.name}</h3>
<Recap
audiences={au}
shouldRenderCritics={au.audienceId === 2}
shouldRenderReaders={au.audienceId === 1}
criticsChildren={renderByAudience(au.audienceId)}
readersChildren={renderByAudience(au.audienceId)}
/>
</div>
))}
</div>
))}
</div>
This is form to render depending on audienceId
return (
<div className="root">
<div>
{props.audienceId === 1 && (
<A
setReader={setReader(readerIdx)}
reader={selectedReader as IreaderState}
/>
)}
</div>
<div>
{props.audienceId === 2 && (
<B
setCritic={setCritic(criticIdx)}
critic={selectedCritic as IcriticState}
/>
)}
</div>
</div>
);
Finally, for each reader/critic, there is a form to input.
export default function A(props: Aprops) {
const handleChange = (
event: ChangeEvent<{ value: number | string; name: string }>
) => {
const { name, value } = event.target;
const r = { ...props.reader };
r[name] = value;
props.setReader(r);
};
return (
<div>
<TextField
name="rate"
type="number"
value={get(props.reader, "rate", "")}
onChange={handleChange}
/>
<TextField
name="feedback"
value={get(props.reader, "feedback", "")}
onChange={handleChange}
/>
</div>
);
The issue
Each time I fill in a field of critic/reader, it set state for that object but also set the initial state for the rest of objects. it only set state of the form in which in focus and doesnt keep the values of other forms
I don't know what causes the problem.
Here is a sandbox reproducing the issue https://codesandbox.io/s/project-u0m5n
Wow, this was a doozy. The ultimate problem stems from the fact that you're calling the following component:
export default function FormsByAudience(props: FormsByAudienceProps) {
const [critics, setCritics] = useCritics(props.books);
const setCritic = (index: number) => (critic: IcriticState) => {
const c = [...critics];
c[index] = critic;
setCritics(c);
};
// ...
// in render method:
setCritic={setCritic(criticIdx)}
for every single different critic (and reader):
props.audiences.critics.map((c) => (
<div key={c.id}>
<div>{c.name}</div>
{props.shouldRenderCritics &&
props.criticsChildren &&
cloneElement(props.criticsChildren, { criticId: c.id })}
</div>
))}
where props.criticsChildren
contains <FormsByAudience audienceId={id} books={books} />
.
As a result, inside a single render, there are lots of separate bindings for the critics
variable. You don't have a single critics
for the whole application, or for a given audience; you have multiple critics
arrays, one for each critic. Setting the state for one critic's critics
array in FormsByAudience
does not result in changes to other critics
arrays that the other critics' React elements close over.
To fix it, given that the critics
array is being created from props.books
, the critics
state should be put near the same level where the books are used, and definitely not inside a component in a nested mapper function. Then pass down the state and the state setters to the children.
The exact same thing applies to the readers
state.
Here is a minimal live Stack Snippet illustrating this problem:
const people = ['bob', 'joe'];
const Person = ({name, i}) => {
const [peopleData, setPeopleData] = React.useState(people.map(() => 0));
console.log(peopleData.join(','));
const onChange = e => {
setPeopleData(
peopleData.map((num, j) => j === i ? e.target.value : num)
)
};
return (
<div>
<div>{name}</div>
<input type="number" value={peopleData[i]} onChange={onChange} />
</div>
);
};
const App = () => {
return people.map(
(name, i) => <Person key={i} {...{ name, i }} />
);
};
ReactDOM.render(<App />, document.querySelector('.react'));
<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<div class='react'></div>
To make the passing of props down easier to type and read, you could consider consolidating the 4 variables (2 data and 2 setters) into a single object.
export default function App() {
const [critics, setCritics] = useCritics(books);
const [readers, setReaders] = useReaders(books);
const dataAndSetters = { critics, setCritics, readers, setReaders };
const renderByAudience = (id: number) => {
return <FormsByAudience audienceId={id} {...{ books, dataAndSetters }} />;
};
Pass dataAndSetters
down until you get to
export default function FormsByAudience(props: FormsByAudienceProps) {
const { critics, setCritics, readers, setReaders } = props.dataAndSetters;
Live demo:
https://codesandbox.io/s/project-forked-woqhf
If I could offer a few other pieces of advice, to up your code quality significantly:
Change Recap
so that FormsByAudience
is imported and called in Recap
instead of in the parent. It's really weird (and hard to easily understand at a glance) for renderByAudience
to create a React element that sometimes won't ever get used.
Recap
has the critics and readers switched around. This is probably unintentional. This:
<h5>Readers</h5>
{!isEmpty(props.audiences.critics) &&
props.audiences.critics.map((c) => (
should probably be
<h5>Readers</h5>
{props.audiences.readers.map((c) => (
(there's no need for an isEmpty
check before mapping an array)
The whole
const selectedReader =
readers.find((r) => r.readerId === (props.readerId as number)) || {};
const readerIdx = readers.indexOf(selectedReader as IreaderState);
const selectedCritic =
critics.find((r) => r.criticId === (props.criticId as number)) || {};
const criticIdx = critics.indexOf(selectedCritic as IcriticState);
is unnecessary. Just pass down the reader/critic index from the parent component instead, and use critics[criticIdx]
to get to the critic in question.
In hook.ts
, this:
const byAudience = books
.map((b) => b.audiences)
.flat()
.filter((bk) => [1].includes(bk.audienceId));
simplifies to
const byAudience = books
.flatMap(b => b.audiences)
.filter(bk => bk.audienceId === 1);
And this:
byAudience.forEach((el) => {
el.readers.map((r) => {
return readersToSet.push({ feedback: "", rate: "", readerId: r.id });
});
return readersToSet;
});
doesn't make sense because forEach
ignores its return value, and .map
should only be used when returning a value from the callback to construct a new array. Either use flatMap
on byAudience
, or push inside a for..of
loop.
const readersToSet = byAudience.flatMap(
el => el.readers.map(
r => ({ feedback: "", rate: "", readerId: r.id })
)
);
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With