Fix attachment modal doesn't close when pressing search shortcut #27639
Conversation
| } | ||
|
|
||
| hideModal(true); |
There was a problem hiding this comment.
I think we should create a ref for hideModal function too, but as it doesn't have any problem yet, I leave it as it is. Let me know if we should make it a ref too.
There was a problem hiding this comment.
I think ref is only needed for variables that are passed by value (not confirmed).
There was a problem hiding this comment.
Maybe, haven't tested it too
src/components/Modal/BaseModal.js
Outdated
| if (!isVisible) { | ||
| return; | ||
| } | ||
| Modal.willAlertModalBecomeVisible(false); | ||
| Modal.setCloseModal(null); |
There was a problem hiding this comment.
I don't think this logic fits best as the cleanup logic. Can we done this using usePrevious instead?
useEffect(() => {
if (isVisible) {
Modal.willAlertModalBecomeVisible(true);
Modal.setCloseModal(onClose);
} else if (wasVisible && !isVisible) {
Modal.willAlertModalBecomeVisible(false);
Modal.setCloseModal(null);
}
}, [isVisible, wasVisible, onClose]);There was a problem hiding this comment.
The cleanup func runs when isVisible changes, so if isVisible is updated to false the cleanup func runs with isVisible value is still true. I must admit it's harder to understand, so I updated it using your suggestion.
| // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu | ||
| Modal.setCloseModal(isVisible ? onClose : null); | ||
| }, [isVisible, onClose]); | ||
| isVisibleRef.current = isVisible; |
There was a problem hiding this comment.
NAB. Is there a significant gain merging the two useEffects together? I think it's more readable to have the isVisibleRef useEffect separated
There was a problem hiding this comment.
Nope, just to reduce loc :). I think it's fine to put it here, we can even put it outside of useEffect, but it will feels like out of place.
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chrome
Mobile Web - Safari
Desktopdesktop.moviOS
Android
|
|
✋ 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/AndrewGable in version: 1.3.72-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.72-11 🚀
|
|
This caused regression: Repro step:
Screen.Recording.2023-10-12.at.6.35.41.PM.mov |
|
@0xmiroslav I think you marked the wrong PR. It is coming from this one instead. |
|
@bernhardoj I confirmed that reverting this PR fixes that bug |
|
@0xmiroslav I also confirmed reverting #28486 fix it.
Notice that it early returns when it is not visible, so That PR also caused this issue #29341. They should correct the |
| // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu | ||
| Modal.setCloseModal(onClose); | ||
| } else if (wasVisible && !isVisible) { | ||
| Modal.willAlertModalBecomeVisible(false); |
There was a problem hiding this comment.
Hi @bernhardoj, we are thinking of moving this line, and the same line from the next useEffect into the hideModal function. Was there any specific reason not to do it in this PR? Just asking so that we won't break any edge case that I'm not aware of...
There was a problem hiding this comment.
I don't think there is any reason but just replicating the previous code.
Details
Pressing search shortcut while having attachment modal showing won't close the attachment modal.
Fixed Issues
$ #27359
PROPOSAL: #27359 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Web/Desktop
Android/iOS/mWeb
Verify that we can still open any modal on the app
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Screen.Recording.2023-09-18.at.14.16.37.mov
Mobile Web - Chrome
Screen.Recording.2023-09-18.at.13.54.37.mov
Mobile Web - Safari
Screen.Recording.2023-09-18.at.13.54.14.mov
Desktop
Screen.Recording.2023-09-18.at.14.18.56.mov
iOS
Screen.Recording.2023-09-18.at.13.53.42.mov
Android
Screen.Recording.2023-09-18.at.14.09.45.mov