close modal when opening new page using keyboard shortcut#15413
Conversation
melvin-bot
Bot
requested review from
aimane-chnaif and
puneetlath
and removed request for
a team
|
@puneetlath @aimane-chnaif One of you needs to 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] |
| componentDidMount() { | ||
| if (!this.props.isVisible) { return; } | ||
| Modal.setModalClose(this.props.onClose); | ||
| } |
There was a problem hiding this comment.
Is this necessary? I think modals are mounted when before they are visible, but maybe this is just to make sure? If so please add a comment explaining why.
There was a problem hiding this comment.
This is a special case for PopoverReportActionContextMenu.
I confirmed that isVisible is already true when component mount, so componentDidUpdate is never called.
| this.hideModal(this.props.isVisible); | ||
|
|
||
| // we don't want to call the onClose on unmount | ||
| Modal.setModalClose(null); |
There was a problem hiding this comment.
This needs an explanation of why
There was a problem hiding this comment.
we don't want to call the onClose on unmount
Is this explanation not enough?
I think this description is actually wrong.
This should be better description:
to handle case of modal unmounted with visible state
|
done feedback |
|
Latest changes look good to me. @pecanoro do you have any more concern before I fill checklist? |
|
@aimane-chnaif All good, unless I forgot something! |
|
I tested with all modals across the app.
keyboard.shortcut.movIn production, when press again, it keeps shortcut modal open
right_docked.mov
centered.movcc: @puneetlath @pecanoro |
|
I would have to test all cases, but after seeing your videos, it seems to work great. I think it does not matter if pressing again the shortcut closes the modal, I actually like it more that way. |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - ChromeMobile Web - SafariDesktopdesktop.moviOSAndroid |
There was a problem hiding this comment.
Actually I think those are not issues at all, same thought as @pecanoro so approving
@puneetlath all yours!
|
Thanks for the thorough testing @aimane-chnaif. I agree that I see no problems with that behavior. |
puneetlath
left a comment
puneetlath
left a comment
There was a problem hiding this comment.
Mostly looks good. I think the comments could be clearer.
Co-authored-by: Puneet Lath <puneet@expensify.com>
puneetlath
left a comment
puneetlath
left a comment
There was a problem hiding this comment.
Seems good to me. @pecanoro do you want to review again?
|
Retested! I think it works great! Unless a missed some edge case! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
github-actions
Bot
added
the
DeployBlockerCash
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.2.77-0 🚀
|
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.2.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.77-4 🚀
|
| } | ||
|
|
||
| Modal.willAlertModalBecomeVisible(this.props.isVisible); | ||
| Modal.setCloseModal(this.props.isVisible ? this.props.onClose : null); |
There was a problem hiding this comment.
@situchan Do you recall why this call is done in that way? i.e. why we need to set the callback to null if this modal is not visible? Why we don't do it the same as componentDidMount:
if (this.props.isVisible) {
Modal.setModalClose(this.props.onClose);
}There was a problem hiding this comment.
We pass onClose callback to setCloseModal to allow other parts of the app to close this modal manually.
If already closed, why we still need to have this stale callback? Also see below code why it's needed:
Lines 17 to 25 in e48121c
Btw, does that cause any issue?
There was a problem hiding this comment.
If already closed, why we still need to have this stale callback?
We may have other modals that did set the callback. If we render a new modal invisible, the old visible modal close callback will be overwritten.
There was a problem hiding this comment.
Btw, does that cause any issue?
Not exactly, but the functional rewrite of the modal seems to be the cause of the regression. I think there was a misunderstanding of how the code is supposed to be executed. Here at this point, componentDidMount and componentDidUpdate call Modal.setModalClose differently (as explained above) but after the functional migration we only use useEffect and both logic get merged to look like componentDidUpdate,
7 participants
Details
Fixed Issues
$ #14889
PROPOSAL: #14889 (comment)
Tests
Same as QA step
Offline tests
Same as QA step
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
desktop.mov
iOS
Android
NOTE: The main videos are in web, desktop. Other platforms are just app-working screenshot since keyboard shortcut is not available.