-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Selecting last possible member to group hides selected members. #34847
fix: Selecting last possible member to group hides selected members. #34847
Conversation
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Facing issues when trying to login in mWeb, will update the screenshots once I can login. |
src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosAndroid: NativemaxParticipantsAndroid.mp4Android: mWeb ChromemaxParticipantsAndroidChrome.mp4maxParticipantsAndroidChrome2.mp4iOS: NativemaxiOSNative.mp4iOS: mWeb SafarimaxParticipantsiOSSafari.mp4MacOS: Chrome / SafarimaxChrome.mp4MacOS: DesktopmaxDesktop.mp4 |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@c3024 all done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please change this step in the Tests to the step you followed - either The search string you used or the step you followed.
Also PR author checklist is failing. Please check all boxes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! A few small suggestions, I think it would be nice to clean up the reused code & think about reordering the params of formatSectionsFromSearchTerm
, but curious to hear what y'all think
@@ -259,10 +260,15 @@ function MoneyRequestParticipantsSelector({ | |||
); | |||
|
|||
// When search term updates we will fetch any reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also update this comment to explain the new changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also since this is pretty much copy / pasted in 3 files, should we make it a reusable lib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should do both.
cc: @Krishna2323
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c3024, comments have been updated, can you help me understanding what do you mean by making it reusable lib, the setSearchTermAndSearchInServer
uses useCallback
& setState
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We extract this
const setSearchTermAndSearchInServer = useCallback(
(text = '') => {
if (text && !maxParticipantsReached) {
Report.searchInServer(text);
}
setSearchTerm(text);
},
[maxParticipantsReached],
);
to a function in OptionListUtils.ts
(or a more appropriate file) and use it in the three pages. @Krishna2323
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c3024, I believe it will over complicate things, as we need to use useCallback because we pass it to OptionsSelector
.
Otherwise the option seems to create a util function that returns a function which will be used inside useCallback but as I said it will complicate things. Current implementation looks simpler to me and its just few lines of code, so I don't think we should extract few lines and still use useCallback which is a must.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a custom hook like this?
import { useCallback } from 'react';
import * as Report from '@userActions/Report';
const useSearchTermAndSearch = (setSearchTerm, maxParticipantsReached) => {
const setSearchTermAndSearchInServer = useCallback(
(text = '') => {
if (text && !maxParticipantsReached) {
Report.searchInServer(text);
}
setSearchTerm(text);
},
[maxParticipantsReached, setSearchTerm]
);
return setSearchTermAndSearchInServer;
};
export default useSearchTermAndSearch;
and use it in the pages like this
const setSearchTermAndSearchInServer = useSearchTermAndSearch(setSearchTerm, maxParticipantsReached);
WDYT? @Beamanator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that plan @c3024 👍 looks like we have a decent amount of custom hooks in the src/hooks
directory, so this would be a nice one to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c3024, can you pls review once, created the hook and tested.
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
…a2323/issue/34111
…a2323/issue/34111
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me! But yes let's test again before we merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again and works well.
selectLastWeb.mp4
selectLastDesktop.mp4
selectLastAndroid.mp4
selectLastiOS.mp4
Errors/warnings in the videos are also on main
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! LGTM!
✋ 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 production by https://github.com/marcaaron in version: 1.4.36-5 🚀
|
Details
Fixed Issues
$ #34111
PROPOSAL: #34111 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4