Fix: Checkmark missing on Assignee Page#74636
Conversation
|
@mananjadhav 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] |
| @@ -222,6 +216,8 @@ function TaskAssigneeSelectorModal() { | |||
| onChangeText={setSearchTerm} | |||
| textInputValue={searchTerm} | |||
| headerMessage={headerMessage} | |||
| initiallyFocusedOptionKey={sections[0].data.find((mode) => mode.isSelected)?.keyForList} | |||
There was a problem hiding this comment.
❌ PERF-4 (docs)
The .find() operation on sections[0].data executes on every render, even when sections hasn't changed. This creates unnecessary computation overhead.
Suggested fix:
Memoize the focused option key:
const initiallyFocusedOptionKey = useMemo(() => {
return sections[0]?.data.find((mode) => mode.isSelected)?.keyForList;
}, [sections]);Then use it in the prop:
<SelectionList
initiallyFocusedOptionKey={initiallyFocusedOptionKey}
// ... other props
/>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Will test this tonight. |
|
@PiyushChandra17 The given tests steps work fine. But the same also needs to be fixed for Edit assignee screen.
assignee-checkmark-on-edit-assignee.mov |
heyjennahay
left a comment
There was a problem hiding this comment.
No concerns with the product change
@mananjadhav I think this is a seperate issue and needs to be fixed seperately because as far as i am concern 
cc: @heyjennahay |
|
@JS00001 What do you think? I think if it's a small fix should be done along in thie same PR. |
|
Yeah since its small, we can do it here |
|
@mananjadhav @JS00001 Cool, will do that first time at the desk tomm. |
|
@mananjadhav I'm working on this, will give the update .. |
|
Thanks for the update @PiyushChandra17. Ping me once ready for QA. |
|
@mananjadhav I have investigated into this, my conclusion:
isSelected={task?.assigneeAccountID === personalDetails?.accountID}Perhaps this doesn't seem to work, needs discussion here |
@mananjadhav I still think, this is a seperate issue since cc: @JS00001 |
|
Thanks for the analysis. Yes if the solution isn't the same, then we can create a separate issue.Keen on what @JS00001 thinks. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-assignee.movAndroid: mWeb Chromemweb-chrome-assignee.moviOS: HybridAppiOS: mWeb Safarimweb-safari-assignee.movMacOS: Chrome / Safariweb-assignee.movMacOS: Desktopdesktop-assignee.mov |
mananjadhav
left a comment
There was a problem hiding this comment.
I have issues with my iOS build. Approved tested on all other platforms. Pending @JS00001 to check this comment.
|
@PiyushChandra17 Could you please post the task bug in #expensify-bugs, and then I'll merge this? |
|
@JS00001 I think i am not able to access my slack channel (already put a formal mail and form regarding this) due to some odd reasons, hence will not be able to report the task bug in #expensify-bugs. Perhaps i think @mananjadhav can do the same. Thanks! |
|
Sure, works for me, @mananjadhav Could you please post the bug in the bug reports channel? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.2.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.2.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.2.60-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.60-2 🚀
|
Explanation of Change
Checkmark missing on Assignee Page
Fixed Issues
$ #73761
PROPOSAL: #73761 (Comment)
Tests
Offline tests
QA Steps
Same as tests
// 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