[CP Staging] Reverts to fix issues on invite member page#72582
Conversation
|
@allroundexperts Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| if (isOptionInList) { | ||
| newSelectedOptions = selectedOptions.filter((selectedOption) => selectedOption.login !== option.login); | ||
| } else { | ||
| newSelectedOptions = [...selectedOptions, {...option, isSelected: true}]; |
There was a problem hiding this comment.
❌ PERF-4
Objects passed as props should be properly memoized to prevent unnecessary re-renders.
The spread operator {...option, isSelected: true} creates a new object on every render, which will cause the child component to re-render even when the actual data hasn't changed.
const memoizedOption = useMemo(() => ({...option, isSelected: true}), [option]);
newSelectedOptions = [...selectedOptions, memoizedOption];| if (isOptionInList) { | ||
| newSelectedOptions = selectedOptions.filter((selectedOption) => selectedOption.login !== option.login); | ||
| } else { | ||
| newSelectedOptions = [...selectedOptions, {...option, isSelected: true}]; |
There was a problem hiding this comment.
❌ PERF-4
Objects passed as props should be properly memoized to prevent unnecessary re-renders.
The spread operator {...option, isSelected: true} creates a new object on every render, which will cause the child component to re-render even when the actual data hasn't changed.
const memoizedOption = useMemo(() => ({...option, isSelected: true}), [option]);
newSelectedOptions = [...selectedOptions, memoizedOption];|
|
||
| const newSelectedOptions: MemberForList[] = []; | ||
| selectedOptions.forEach((option) => { | ||
| newSelectedOptions.push(option.login && option.login in detailsMap ? {...detailsMap[option.login], isSelected: true} : option); |
There was a problem hiding this comment.
❌ PERF-4
Objects passed as props should be properly memoized to prevent unnecessary re-renders.
The spread operator {...detailsMap[option.login], isSelected: true} creates a new object on every render within the forEach loop, which can cause performance issues.
const memoizedOption = useMemo(() =>
option.login && option.login in detailsMap
? {...detailsMap[option.login], isSelected: true}
: option,
[option, detailsMap]
);
newSelectedOptions.push(memoizedOption);| }); | ||
| } | ||
| selectedOptions.forEach((option) => { | ||
| newSelectedOptions.push(option.login && option.login in detailsMap ? {...detailsMap[option.login], isSelected: true} : option); |
There was a problem hiding this comment.
❌ PERF-4
Objects passed as props should be properly memoized to prevent unnecessary re-renders.
The spread operator {...detailsMap[option.login], isSelected: true} creates a new object on every render within the forEach loop, which can cause performance issues.
const memoizedOption = useMemo(() =>
option.login && option.login in detailsMap
? {...detailsMap[option.login], isSelected: true}
: option,
[option, detailsMap]
);
newSelectedOptions.push(memoizedOption);|
@lakchote looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Not an emergency, straight revert. |
[CP Staging] Reverts to fix issues on invite member page (cherry picked from commit 8996c60) (cherry-picked to staging by mountiny)
Codecov Report❌ Patch coverage is
... and 20 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@mountiny do we need to check linked issues? |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.2.30-4 🚀
|
|
We reverted this, so all good |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.30-6 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.2.31-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.31-2 🚀
|
Explanation of Change
Straight revert off #72534 and #71455 to fix 2 blockers
Fixed Issues
$ #72523
$ #72516
PROPOSAL:
Tests
Same as the linked blockers
Offline tests
Same as the linked blockers
QA Steps
Same as the linked blockers
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop