chore: Remove refs from Rich Editor Data Field#4569
chore: Remove refs from Rich Editor Data Field#4569OliwiaGowor merged 4 commits intokyma-project:mainfrom
Conversation
dbadura
requested review from
KonradPietocha,
OliwiaGowor,
akucharska,
awoloszyn22,
chriskari and
mrCherry97
as code owners
|
|
||
| function createInternalState(data, previousInitialState) { | ||
| const dataAsArray = transformData(data, previousInitialState); | ||
| if (checkIfLastItemIsNotNull) { |
There was a problem hiding this comment.
Is this working correctly? You're not executing checkIfLastItemIsNotNull function here and not passing it any arguments. In such form it will be always true.
| const [internalData, setInternalData] = useState([]); | ||
| const valueRef = useRef(null); | ||
| const firstRender = useRef(true); // detect languages only on first render | ||
| const [internalData, setInternalData] = useState(() => |
There was a problem hiding this comment.
Will this function execute here? Shouldn't it be: createInternalState(data, null) instead of: ()=>createInternalState(data, null)?
There was a problem hiding this comment.
In this form the createInternalState will be called only on the initial render and the result will be saved, instead of calling the function on every render
There was a problem hiding this comment.
Thank you. Great thing. I read later that this is indeed very useful when the initial state is expensive to process.
| @@ -53,15 +64,15 @@ export function RichEditorDataField({ | |||
| key={index} | |||
| item={item} | |||
| setInternalData={setInternalData} | |||
There was a problem hiding this comment.
It's strange but I can't see setInternalData prop in RichEditorSection component. Someone probably changed it at some point but left it.
| // update internal value | ||
| if (JSON.stringify(data) !== valueRef.current) { | ||
| setInternalData( | ||
| Object.entries(data || {}).map(([key, value]) => ({ | ||
| key, | ||
| value, | ||
| language: internalData.find((d) => d?.key === key)?.language || '', | ||
| })), | ||
| ); | ||
| firstRender.current = false; | ||
| valueRef.current = JSON.stringify(data); |
There was a problem hiding this comment.
Maybe it would have been enough to put it in useEffect?
There was a problem hiding this comment.
useEffect would trigger an extra render cycle here, React docs actually recommend this pattern over useEffect
4 participants
Description
Changes proposed in this pull request:
Related issue(s)
#4396
Definition of done
backlog#4567