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

fix: Properly dismiss RCTDevLoadingView #2211

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

Saadnajmi
Copy link
Collaborator

Summary:

This is attempt #2 to fix issues with the refactor of RCTDevLoadingView , the first being #2201 . Namely, the RCTDevLoadingView sheet doesn't always dismiss, and sticks around blocking the app.

This PR addresses two main things:

  1. React Native seems to not balance its' own calls to [RCTDevLoadingView showMessage] and [RCTDevLoadingView hide]. As a result, we might end up calling beginSheet more times than we call endSheet. My theory is we're ending up with multiple sheets/modals, which is why the sheet is sticking around. We can fix this by only calling beginSheet if we aren't already presented.

  2. Digging around a couple of places in StackOverflow, it seems it's also important to call [mySheetWindow orderOut:] I the beginSheet completion handler to make the sheet go away. In my testing, this seems to be true.

While we're here, let's fix a couple of nits:

  1. Release the sheet window when closed. I don't think there's any benefit keeping it around, especially if we're already nilling it out in hide.
  2. Set the font of the label to be the same as iOS.

Test Plan:

Launching the app a bunch of times (sometimes with breakpoints set to interrupt the flow), I seem to not have the sheet stick around. Of course this is intermittent, so it's not a definitive test.

@Saadnajmi Saadnajmi merged commit 3ebf897 into microsoft:main Oct 3, 2024
11 of 13 checks passed
@Saadnajmi Saadnajmi deleted the loadingview branch October 3, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants