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

Use refs for modals #1200

Merged
merged 1 commit into from
May 30, 2024
Merged

Use refs for modals #1200

merged 1 commit into from
May 30, 2024

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented May 29, 2024

Description

This fixes concurrent showModal calls with stale values which lead to modals getting replaced instead of stacked.

fixes #1194 (comment)

Additional Context

Checklist

Are your changes backwards compatible? Please answer below:

Yes

Did you QA this? Could we deploy this straight to production? Please answer below:

Searched for showModal calls and thus tested modals for QR codes, custom zaps, zoomable images, avatar upload, delete, anon info, "you will lose your work"-modal. There are more but looks good to me

For frontend changes: Tested on mobile? Please answer below:

Did you introduce any new environment variables? If so, call them out explicitly here:

@ekzyis ekzyis added the bug label May 29, 2024
This fixes concurrent showModal calls with stale values which lead to modals getting replaced instead of stacked.
@huumn
Copy link
Member

huumn commented May 29, 2024

Curious. How does this fix the issue described in the comment? That issue was unrelated to modals afaict

@ekzyis
Copy link
Member Author

ekzyis commented May 29, 2024

Curious. How does this fix the issue described in the comment? That issue was unrelated to modals afaict

It's related to modals because the payments weren't lost but their errors opened up a modal but you never saw all of them because only the modal for the most recent error was shown. The previous modals were simply replaced without running the onClose of the previous ones. So it only looked like they were lost, but they were still pending.1

Footnotes

  1. But they should get marked as failed with "payment was interrupted" as the reason

@huumn huumn merged commit 6047c37 into master May 30, 2024
6 checks passed
@huumn huumn deleted the modal-ref branch May 30, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants