forked from facebook/react-native
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Refactor Redbox, Logbox, and DevLoadingView as Sheets #2151
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hanges (facebook#41217) Summary: When opening `RCTRedBox` on an iPad (and also visionOS) there was an issue with buttons width going out of screen. When changing screen orientation, RedBox wasn't recalculating view positions. **Root cause**: Getting frame of root view to display this modal and basing all calculations on it. **Solution**: Use Auto Layout to build UI that responds to orientation changes and device specific modal presentation. I've also tested it with adding custom buttons to RedBox and it works properly. [IOS] [FIXED] - adjust RCTRedBox to work for iPad and support orientation changes Pull Request resolved: facebook#41217 Test Plan: Launch the app without metro running and check out RedBox that's shown there. Also change screen orientation to see proper recalculation of view positions. https://github.com/facebook/react-native/assets/52801365/892dcfe7-246f-4f36-be37-12c139c207ac https://github.com/facebook/react-native/assets/52801365/dfd0c3d8-5997-462d-97ec-dcc3de452e26 Reviewed By: GijsWeterings Differential Revision: D50734569 Pulled By: javache fbshipit-source-id: 51b854a47caf90ae46fcd32c4adcc64ec2ceb63f
) Summary: This PR is a continuation of my previous PR where I refactored RCTRedBox to use Auto Layout (facebook#41217). This PR uses less verbose API for defining constraints. ## Changelog: [IOS] [CHANGED] - use less verbose Auto Layout API for RCTRedBox constraints Pull Request resolved: facebook#42261 Test Plan: Launch the app without metro enabled to see the RCTRedBox ![CleanShot 2024-01-12 at 14 54 20@2x](https://github.com/facebook/react-native/assets/52801365/32ee9916-3e32-46c3-9f6b-c313631aa1e5) ![CleanShot 2024-01-12 at 14 54 16@2x](https://github.com/facebook/react-native/assets/52801365/c625b9b9-b462-4e67-831f-0192427bbe93) Reviewed By: NickGerleman Differential Revision: D52730458 Pulled By: javache fbshipit-source-id: dc7227e7b6e3238c195342cb0460850b57eb75c3
Saadnajmi
force-pushed
the
redbox
branch
2 times, most recently
from
July 31, 2024 07:06
d00e06b
to
f04a530
Compare
JunielKatarn
approved these changes
Aug 1, 2024
amgleitman
approved these changes
Aug 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Upstream in React Native 0.74, RCTRedbox got heavily rewritten (see facebook@e2eb26c and facebook@b311c3e) . As such, we needed to update our macOS implementation. This proved to be quite difficult, as NSTableView is quite... obtuse. Many thanks to @FalseLobster for doing much of that part of the rewrite. I am cherry picking the rewrite commits here so we can reduce churn on the larger "0.74 merge" PR.
While we are touching this code, I realized that a natural presentation pattern for the red box would be to use a sheet. In fact, that would also apply to RCTDevLoadingView (which is particularly annoying as it's presented as a fixed-in-place tiny window on your screen) and RCTLogBox. So... I went ahead and refactored all three to use sheets. The result is quite nice!
While here, I realized separately,
RCTRootView
has aloadingView
property so you can set something like an ActivityIndicator while it loads JS. There was some modifications to get that working, So I'm also adding that in this PR.Test Plan:
RCTRedBox rewritten, and presented as a sheet:
Screen.Recording.2024-07-30.at.11.26.54.PM.mov
RCTDevLoadingView and RCTLogBox presented as sheets:
Screen.Recording.2024-07-30.at.11.29.19.PM.mov
RCTRootView.loadingView
set to anNSProgressIndicator
using the code block below. Ignore the devLoadingView looking different, I was testing a different design.Screen.Recording.2024-07-31.at.5.29.45.PM.mov