feat(ios): RCTViewControllerAppearanceListener#56598
feat(ios): RCTViewControllerAppearanceListener#56598hannojg wants to merge 5 commits intofacebook:mainfrom
Conversation
cipolleschi
left a comment
There was a problem hiding this comment.
@hannojg Thanks for working on this, but unfortunately we can't merge this as it is. Adding an RCTViewController base class is a huge breaking change as it implies that all the view controller used by react native must inherit from the RCTViewController. It means that if some user customize the createRootViewController function by returning something different (eg. a UITTabController), they are going to lose all the features that you are pushing with RCTViewController.
We should probably find a better way to handle this that does not requires a new base class for ViewControllers.
|
Warning JavaScript API change detected This PR commits an update to
This change was flagged as: |
|
Hey @cipolleschi , okay, i see! I don't think this base class is needed. I added a category on UIViewController. This way you don't need to subclass the @implementation MyViewController
- (void)viewDidAppear:(BOOL)animated
{
[super viewDidAppear:animated];
[self reactNotifyViewControllerDidAppear:animated];
}how does that sound? |
WoLewicki
left a comment
There was a problem hiding this comment.
Just some comments for now, looks reasonable.
| - (RCTViewControllerAppearanceState *)reactViewControllerAppearanceState | ||
| { | ||
| RCTViewControllerAppearanceState *state = | ||
| objc_getAssociatedObject(self, @selector(reactViewControllerAppearanceState)); |
There was a problem hiding this comment.
We're doing it in such weird way cause its category right?
| RCTViewControllerAppearanceState *state = [self reactViewControllerAppearanceState]; | ||
| [state.listeners addObject:listener]; | ||
|
|
||
| if (state.visible && [listener respondsToSelector:@selector(reactViewControllerDidAppear:animated:)]) { |
There was a problem hiding this comment.
Do we need to check the selector? Guess that if it does not implement it then it will just fail silently right?
There was a problem hiding this comment.
Also, why are we dispatching something in the registration method?
There was a problem hiding this comment.
Do we need to check the selector?
Yes, or are we going to get not recognized selector error otherwise.
Also, why are we dispatching something in the registration method
Are you worried that we might not catch all the events?
There was a problem hiding this comment.
yes, we need the check, otherwise we would get unrecognized selector exception i think 🤔
why are we dispatching something in the registration method?
I think there are two ways to handle this with listener patterns. The question is basically, what should happen if you attach a listener for something that has already happened. Should the listener callback be invoked immediately (the case here), or is it the callers responsibility to check for the condition first (and its for future events only).
I felt like its easier to just send it here so the caller doesn't have to do extra checks + the event is view_Did_Appear, so I felt its reasonable to fire it when the event has already happened. Plus i think it will just make it simpler for the calling site
There was a problem hiding this comment.
// Edit: oops, only saw Riccardo's comment now
|
This is much better, thanks for working on this. |
|
I added an example into the summary, but also made a change to use this pattern for react-native's root view controller: Not sure though how important it is for react-native core, as this only gets relevant if you use multiple view controllers. I will now make two other PRs:
That should give us the full picture of how all of that works together! |
Summary:
This is the first step to add support in react-native for views to listen to their parent
UIViewController viewDidAppear:event.Proposing this as part of:
autoFocuscalled indidMoveToWindowcausing transition glitches with UIKit #56595Example
Some native view component potentially want to hook into the UIViewController's
viewDidAppear(_:)event to run some logic (e.g. a TextInput may wants to apply its auto focus logic at this point).For a UIViewController in react-native to support this it has to opt into it, by implementing
viewDidAppear:A view component interested in that event can then add a listener, which will be called (either in the future, or immediately if the VC already appeared):
For an example see also changing react-native's root controller: #56618
Changelog:
[IOS] [ADDED] - Add support to listen to
UIViewController viewDidAppear:&viewDidDisappear:Test Plan:
Test that RN tester is still compiling and working as usual