-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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: 29378 Distance - Search is open in background on distance image preview #30556
Conversation
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've noticed some issues in the parts of our app where we use setCloseModal. It seems like it’s not working correctly in all cases.
Also, I’m confused about why we are using a key and a count number for closing modals. I can’t find where we really need the count number or how it helps us. Can someone explain why we use this system? I think maybe we can make it simpler by rewriting closeModals object to an array.
src/libs/actions/Modal.ts
Outdated
/** | ||
* Get the available key that we can store the onClose callback into it | ||
*/ | ||
function getAvailableKey() { |
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.
This function increases a number to make new keys. This could cause problems if we make a lot of keys over time or if many things happen at once.
@ArekChr Thanks for your review. If we use the array, how can we clear the onClose function when the modal is hidden? |
@tienifr I think the |
@ArekChr Thanks for your suggestion. I just update the PR. Thanks |
@tienifr I've noticed a regression issue with shortcuts: CMD+K or CMD+J don't open the chat or shortcuts menu as they should and only work after switching chats. On the main branch it works as expected. |
Ah, my bad. I forgot the case when closeModals is empty. We already implemented that case before, and I just did the same. Thanks for your comment. I updated |
src/libs/actions/Modal.ts
Outdated
const reverseCloseModals = [...closeModals].reverse(); | ||
reverseCloseModals.forEach((onClose) => { |
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.
nit.
const reverseCloseModals = [...closeModals].reverse(); | |
reverseCloseModals.forEach((onClose) => { | |
[...closeModals].reverse().forEach((onClose) => { |
onOpenCallback: () => Modal.setCloseModal(() => props.onClose(props.anchorRef)), | ||
onCloseCallback: removeOnClose.current, | ||
onOpenCallback: () => { | ||
removeOnClose.current(); |
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.
Is this needed? Is this workaround associated with the logic for opening and closing modals?
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.
Before saving onClose, we need to remove the previous one right? Although we have the logic to prevent saving duplicate onClose but we just compare the reference. It's not really correct because the reference can be changed (even they have the same mean).
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb.chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb.safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
@ArekChr Ah I see, we can move the removeOnClose ref to useEffect and call it in cleanup function |
@ArekChr Thanks for pointing that out. Updated |
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.
All works, skipped mobile screenshots as we don't use keyboard shortcuts here, thanks!
@pecanoro Uploaded videos for all mobile platforms. Let me know if we can test more modal use cases to ensure everything works properly. |
✋ 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/pecanoro in version: 1.4.2-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.2-3 🚀
|
if (props.isVisible) { | ||
props.onModalShow(); | ||
onOpen({ | ||
ref: props.withoutOverlayRef, | ||
close: props.onClose, | ||
anchorRef: props.anchorRef, | ||
onCloseCallback: () => Modal.setCloseModal(null), | ||
onOpenCallback: () => Modal.setCloseModal(() => props.onClose(props.anchorRef)), |
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.
Coming from #32743, it looks like we haven't removed these two callbacks completely, so it caused that bug.
Details
Fixed Issues
$ #29378
PROPOSAL: #29378 (comment)
Tests
Offline tests
Same as above
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)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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: mWeb Chrome
Screenrecorder-2023-10-29-18-02-59-704.mp4
iOS: Native
iOS: mWeb Safari
Screen.Recording.2023-10-29.at.18.02.27.mov
MacOS: Chrome / Safari
Screen.Recording.2023-10-29.at.17.55.06.mov
MacOS: Desktop
Screen.Recording.2023-10-29.at.18.19.44.mov