Skip to content
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

lightbox: Replace onDismiss with await showErrorDialog #937

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Sep 12, 2024

showDialog returns a Future that resolves when the dialog is dismissed.

This refactors showErrorDialog to stop overriding onPressed and barrierIsDismissable, and make the caller call Navigator.pop one more time once the dialog is closed.

Note that we check for mounted again because there has been an asynchronous gap.

See also:
#587 (comment)

@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Sep 12, 2024
@chrisbobbe
Copy link
Collaborator

LGTM assuming you've tested this manually; marking for Greg's review.

@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Sep 19, 2024
@chrisbobbe chrisbobbe requested review from gnprice and removed request for chrisbobbe September 19, 2024 01:49
@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Sep 19, 2024
@PIG208
Copy link
Member Author

PIG208 commented Sep 19, 2024

I forgot to mention that this shouldn't be an nfc change, because the user can close the dialog by tapping the barrier now. Pushed an update to the commit message to reflect that.

@chrisbobbe
Copy link
Collaborator

Makes sense. Ah and I think barrierIsDismissable in the commit message is a typo for barrierDismissable.

`showDialog` returns a `Future` that resolves when the dialog is
dismissed.

This refactors `showErrorDialog` to stop overriding `onPressed` and
`barrierDismissible`, and make the caller call `Navigator.pop` one
more time once the dialog is closed.

This is not an nfc because now the user can tap the barrier (darkened
area around the dialog) to close it.

Note that we check for `mounted` again because there has been an
asynchronous gap.

See also:
  zulip#587 (comment)

Signed-off-by: Zixuan James Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants