Skip to content

fix: call onModalHide when the confirm modal is closed #18897

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 14, 2023
Merged

fix: call onModalHide when the confirm modal is closed #18897

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b8e4bf
call onModalHide when the confirm modal is closed
akinwale May 13, 2023
f5453be
cleanup naming
akinwale May 14, 2023
117f603
use anonymous event handler for onModalHide
akinwale May 14, 2023
e782a78
use AttachmentModal's onModalShow to set the attachmentPreviewActive …
akinwale May 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/components/AttachmentModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ class AttachmentModal extends PureComponent {
*/
closeConfirmModal() {
this.setState({isAttachmentInvalid: false});
if (this.props.onModalHide) {
this.props.onModalHide();
}
Copy link
Member

@parasharrajat parasharrajat May 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad logic... This callback should not be called with confirmModal. This will cause issues in the future. onModalHide is for the main Attachment preview Modal. Let's find another way of fixing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we expose the onConfirm/onCancel for the ConfirmModal as props to the AttachmentModal? The AttachmentModal is responsible for displaying the ConfirmModal, and it's only ever displayed if the attachment is invalid.

Copy link
Member

@parasharrajat parasharrajat May 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we pass a prop called onModalShow to AttachmentModal and call it on the innerModal's onModalShow. Now set the isAttachmentPreviewActive in this callback from ReportActionCompose

Copy link
Member

@rushatgabhane rushatgabhane May 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat let's discuss this deploy blocker on slack for a quick resolution

https://expensify.slack.com/archives/C01GTK53T8Q/p1684066546167389?thread_ts=1683990789.327809&cid=C01GTK53T8Q

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we pass a prop called onModalShow to AttachmentModal and call it on the innerModal's onModalShow. Now set the isAttachmentPreviewActive in this callback from ReportActionCompose

This sounds like it could work. I'll test it out.

Copy link
Contributor Author

@akinwale akinwale May 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane I've implemented @parasharrajat's suggestion and it works great. Pushed up a new commit for review.

}

/**
Expand Down
20 changes: 10 additions & 10 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ class ReportActionCompose extends React.Component {
this.showPopoverMenu = this.showPopoverMenu.bind(this);
this.comment = props.comment;
this.setShouldBlockEmojiCalcToFalse = this.setShouldBlockEmojiCalcToFalse.bind(this);
this.attachmentPreviewClosed = this.attachmentPreviewClosed.bind(this);
this.onAttachmentPreviewClosed = this.onAttachmentPreviewClosed.bind(this);

// React Native will retain focus on an input for native devices but web/mWeb behave differently so we have some focus management
// code that will refocus the compose input after a user closes a modal or some other actions, see usage of ReportActionComposeFocusManager
Expand Down Expand Up @@ -306,6 +306,14 @@ class ReportActionCompose extends React.Component {
this.calculateMentionSuggestion();
}

/**
* Event handler to update the state after the attachment preview is closed.
*/
onAttachmentPreviewClosed() {
this.setShouldBlockEmojiCalcToFalse();
this.setState({isAttachmentPreviewActive: false});
}

getDefaultSuggestionsValues() {
return {
suggestedEmojis: [],
Expand Down Expand Up @@ -856,14 +864,6 @@ class ReportActionCompose extends React.Component {
return true;
}

/**
* Event handler to update the state after the attachment preview is closed.
*/
attachmentPreviewClosed() {
this.setShouldBlockEmojiCalcToFalse();
this.setState({isAttachmentPreviewActive: false});
}

render() {
const reportParticipants = _.without(lodashGet(this.props.report, 'participants', []), this.props.currentUserPersonalDetails.login);
const participantsWithoutExpensifyEmails = _.difference(reportParticipants, CONST.EXPENSIFY_EMAILS);
Expand Down Expand Up @@ -903,7 +903,7 @@ class ReportActionCompose extends React.Component {
<AttachmentModal
headerTitle={this.props.translate('reportActionCompose.sendAttachment')}
onConfirm={this.addAttachment}
onModalHide={this.attachmentPreviewClosed}
onModalHide={this.onAttachmentPreviewClosed}
>
{({displayFileInModal}) => (
<>
Expand Down