Skip to content

feat: allow custom assignment of rootView to rootViewController - #37873

Closed
Gregoirevda wants to merge 4 commits into
react:mainfrom
Gregoirevda:master
Closed

feat: allow custom assignment of rootView to rootViewController#37873
Gregoirevda wants to merge 4 commits into
react:mainfrom
Gregoirevda:master

Conversation

@Gregoirevda

@GregoirevdaGregoirevda commented Jun 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Summary:

To use a native Drawer on iPad, I can override createRootViewController to create a UISplitViewController instead of a UIViewController, but I then need to assign the rootView with

[splitViewController setViewController:mainVC forColumn:UISplitViewControllerColumnSecondary];

which I can currently only do by copy pasting the entire didFinishLaunchingWithOptions and only replacing the assignment

 rootViewController.view = rootView;

In an attempt of making it easier for developers to use a native drawer in iOS, being able to override the assignment would make it easier.

Changelog:

[iOS] [ADDED] - added override method with default implementation

Test Plan

Tested on iPad iOS 16 simulator

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 14, 2023
@analysis-bot

analysis-bot commented Jun 14, 2023•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,755,253-2,417
androidhermesarmeabi-v7a8,067,827-2,409
androidhermesx869,247,854-2,410
androidhermesx86_649,096,988-2,417
androidjscarm64-v8a9,316,459-2,412
androidjscarmeabi-v7a8,506,358-2,414
androidjscx869,379,947-2,416
androidjscx86_649,633,190-2,416

Base commit: b0485be
Branch: main

cipolleschi
cipolleschi requested changes Jun 14, 2023
View reviewed changes

@cipolleschicipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @Gregoirevda! Thank you so much for this PR. I'm super excited that you took some time to add this new feature to RCTAppDelegate. 😄

I left a few comments to align it more to standard iOS conventions, but overall the approach looks good to me!

Comment threadpackages/react-native/Libraries/AppDelegate/RCTAppDelegate.h Outdated
Comment threadpackages/react-native/Libraries/AppDelegate/RCTAppDelegate.h
Comment threadpackages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm Outdated
Gregoirevdaand others added 3 commits June 14, 2023 23:21
@Gregoirevda

Gregoirevda commented Jun 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Hi @cipolleschi
Thanks for the review, should be fine now with changes applied

cipolleschi
cipolleschi approved these changes Jun 15, 2023
View reviewed changes

@cipolleschicipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks good to me! Thanks again for the enhancement!

@facebook-github-bot

facebook-github-bot commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@github-actions

github-actionsBot commented Jun 15, 2023

Copy link
Copy Markdown

This pull request was successfully merged by @Gregoirevda in abc6e1c.

When will my fix make it into a release? | Upcoming Releases

@github-actionsgithub-actionsBot added the Merged This PR has been merged. label Jun 15, 2023
@facebook-github-bot

facebook-github-bot commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in abc6e1c.

@GregoirevdaGregoirevda mentioned this pull request Jun 15, 2023
Closed
adamaveray added a commit to adamaveray/react-native that referenced this pull request Jun 16, 2023
@adamaveray
Merge branch 'main' into feature/automaticKeyboardInsetsImprovements
9d7205f
* main: (135 commits)
translation auto-update for i18n/twilight.config.json on master
Interop: Introduce Bridge proxy
Remove okhttp internal util usage (react#37843)
Update debian to fix CI while updating Node (react#37841)
fix: foreground ripple crash on api < 23 (react#37901)
Re-add the top level LICENSE file (react#37916)
Deploy 0.209.0 to xplat (react#37921)
Re-enable direct debugging with JSC on iOS 16.4+ (react#37914)
add emitObjectProp in parser primitives (react#37904)
Make React-utils its own pod (react#37659)
feat: allow custom assignment of rootView to rootViewController (react#37873)
Switch xplat prettier config to hermes plugin (react#37915)
Set iOS AppState to inactive when app is launching (react#37690)
Use `fileExists` in replace_hermes script (react#37911)
(docs): fix license url (react#37909)
Revert D46719890: Re-enable direct debugging with JSC on iOS 16.4+
Re-enable direct debugging with JSC on iOS 16.4+ (react#37874)
Fix component type references in xplat (react#37903)
Remove usage of passthroughAnimatedPropExplicitValues in ScrollViewStickyHeader (react#37867)
test runtime lifecycle callback (react#37897)
...
@GregoirevdaGregoirevda mentioned this pull request Jun 16, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cipolleschicipolleschicipolleschi approved these changes

Assignees

No one assigned

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Gregoirevda@analysis-bot@facebook-github-bot@cipolleschi