Skip to content

Commit

Permalink
Fix button with react component doesn't get unmounted (#5457)
Browse files Browse the repository at this point in the history
* Fix button with react component doesn't get unmounted

* Test e2e only on iOS
  • Loading branch information
yogevbd committed Sep 5, 2019
1 parent 9faf458 commit 65dde34
Show file tree
Hide file tree
Showing 13 changed files with 87 additions and 6 deletions.
6 changes: 6 additions & 0 deletions e2e/Options.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ describe('Options', () => {
await expect(elementByLabel('Styling Options')).toBeVisible();
});

it(':ios: Reseting buttons should unmount button react view', async () => {
await elementById(TestIDs.SHOW_LIFECYCLE_BTN).tap();
await elementById(TestIDs.RESET_BUTTONS).tap();
await expect(elementByLabel('Button component unmounted')).toBeVisible();
});

xit('hides topBar onScroll down and shows it on scroll up', async () => {
await elementById(TestIDs.PUSH_OPTIONS_BUTTON).tap();
await elementById(TestIDs.SCROLLVIEW_SCREEN_BUTTON).tap();
Expand Down
2 changes: 1 addition & 1 deletion lib/ios/RNNNavigationButtons.m
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ -(RNNUIBarButtonItem*)buildButton: (NSDictionary*)dictionary defaultStyle:(RNNBu
componentOptions.name = [[Text alloc] initWithValue:component[@"name"]];

RNNReactView *view = [_componentRegistry createComponentIfNotExists:componentOptions parentComponentId:self.viewController.layoutInfo.componentId reactViewReadyBlock:nil];
barButtonItem = [[RNNUIBarButtonItem alloc] init:buttonId withCustomView:view];
barButtonItem = [[RNNUIBarButtonItem alloc] init:buttonId withCustomView:view componentRegistry:_componentRegistry];
} else if (iconImage) {
barButtonItem = [[RNNUIBarButtonItem alloc] init:buttonId withIcon:iconImage];
} else if (title) {
Expand Down
2 changes: 2 additions & 0 deletions lib/ios/RNNReactComponentRegistry.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

- (void)removeComponent:(NSString *)componentId;

- (void)removeChildComponent:(NSString *)childId;

- (void)clearComponentsForParentId:(NSString *)parentComponentId;

- (void)clear;
Expand Down
10 changes: 10 additions & 0 deletions lib/ios/RNNReactComponentRegistry.m
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ - (void)removeComponent:(NSString *)componentId {
}
}

- (void)removeChildComponent:(NSString *)childId {
NSMutableDictionary* parent;
while ((parent = _componentStore.objectEnumerator.nextObject)) {
if ([parent objectForKey:childId]) {
[parent removeObjectForKey:childId];
return;
}
}
}

- (void)clear {
[_componentStore removeAllObjects];
}
Expand Down
2 changes: 2 additions & 0 deletions lib/ios/RNNReactView.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,6 @@ typedef void (^RNNReactViewReadyCompletionBlock)(void);

- (void)setAlignment:(NSString *)alignment inFrame:(CGRect)frame;

- (NSString *)componentId;

@end
6 changes: 5 additions & 1 deletion lib/ios/RNNReactView.m
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,17 @@ - (void)contentDidAppear:(NSNotification *)notification {

RNNReactView* appearedView = notification.object;

if (_reactViewReadyBlock && [appearedView.appProperties[@"componentId"] isEqual:self.appProperties[@"componentId"]]) {
if (_reactViewReadyBlock && [appearedView.appProperties[@"componentId"] isEqual:self.componentId]) {
_reactViewReadyBlock();
_reactViewReadyBlock = nil;
[[NSNotificationCenter defaultCenter] removeObserver:self];
}
}

- (NSString *)componentId {
return self.appProperties[@"componentId"];
}

- (void)setRootViewDidChangeIntrinsicSize:(void (^)(CGSize))rootViewDidChangeIntrinsicSize {
_rootViewDidChangeIntrinsicSize = rootViewDidChangeIntrinsicSize;
self.delegate = self;
Expand Down
3 changes: 2 additions & 1 deletion lib/ios/RNNUIBarButtonItem.h
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
#import <Foundation/Foundation.h>
#import <React/RCTRootView.h>
#import <React/RCTRootViewDelegate.h>
#import "RNNReactComponentRegistry.h"

@interface RNNUIBarButtonItem : UIBarButtonItem <RCTRootViewDelegate>

@property (nonatomic, strong) NSString* buttonId;

-(instancetype)init:(NSString*)buttonId withIcon:(UIImage*)iconImage;
-(instancetype)init:(NSString*)buttonId withTitle:(NSString*)title;
-(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView*)reactView;
-(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView *)reactView componentRegistry:(RNNReactComponentRegistry *)componentRegistry;
-(instancetype)init:(NSString*)buttonId withSystemItem:(NSString*)systemItemName;

@end
Expand Down
11 changes: 10 additions & 1 deletion lib/ios/RNNUIBarButtonItem.m
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ @interface RNNUIBarButtonItem ()

@property (nonatomic, strong) NSLayoutConstraint *widthConstraint;
@property (nonatomic, strong) NSLayoutConstraint *heightConstraint;
@property (nonatomic, weak) RNNReactComponentRegistry *componentRegistry;

@end

Expand All @@ -28,9 +29,10 @@ -(instancetype)init:(NSString*)buttonId withTitle:(NSString*)title {
return self;
}

-(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView *)reactView {
-(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView *)reactView componentRegistry:(RNNReactComponentRegistry *)componentRegistry {
self = [super initWithCustomView:reactView];

self.componentRegistry = componentRegistry;
reactView.sizeFlexibility = RCTRootViewSizeFlexibilityWidthAndHeight;
reactView.delegate = self;
reactView.backgroundColor = [UIColor clearColor];
Expand Down Expand Up @@ -76,4 +78,11 @@ - (void)onButtonPressed {
afterDelay:0];
}

- (void)dealloc {
if ([self.customView isKindOfClass:[RNNReactView class]]) {
RNNReactView* customView = self.customView;
[self.componentRegistry removeChildComponent:customView.componentId];
}
}

@end
9 changes: 9 additions & 0 deletions playground/src/screens/LifecycleButton.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const RoundedButton = require('./RoundedButton');

class LifecycleButton extends RoundedButton {
componentWillUnmount() {
alert('Button component unmounted');
}
}

module.exports = LifecycleButton;
33 changes: 31 additions & 2 deletions playground/src/screens/OptionsScreen.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const React = require('react');
const { Component } = require('react');
const {Component} = require('react');
const Root = require('../components/Root');
const Button = require('../components/Button')
const Navigation = require('../services/Navigation');
Expand All @@ -16,7 +16,9 @@ const {
PUSH_BTN,
HIDE_TOPBAR_DEFAULT_OPTIONS,
SHOW_YELLOW_BOX_BTN,
SET_REACT_TITLE_VIEW
SET_REACT_TITLE_VIEW,
RESET_BUTTONS,
SHOW_LIFECYCLE_BTN
} = require('../testIDs');

class Options extends Component {
Expand Down Expand Up @@ -68,10 +70,37 @@ class Options extends Component {
<Button label='Hide TopBar in DefaultOptions' testID={HIDE_TOPBAR_DEFAULT_OPTIONS} onPress={this.hideTopBarInDefaultOptions} />
<Button label='Set React Title View' testID={SET_REACT_TITLE_VIEW} onPress={this.setReactTitleView} />
<Button label='Show Yellow Box' testID={SHOW_YELLOW_BOX_BTN} onPress={() => console.warn('Yellow Box')} />
<Button label='StatusBar' onPress={this.statusBarScreen} />
<Button label='Show Lifecycle button' testID={SHOW_LIFECYCLE_BTN} onPress={this.showLifecycleButton} />
<Button label='Remove all buttons' testID={RESET_BUTTONS} onPress={this.resetButtons} />
</Root>
);
}

showLifecycleButton = () => Navigation.mergeOptions(this, {
topBar: {
rightButtons: [
{
id: 'ROUND',
testID: ROUND_BUTTON,
component: {
name: Screens.LifecycleButton,
passProps: {
title: 'Two'
}
}
}
]
}
});

resetButtons = () => Navigation.mergeOptions(this, {
topBar: {
rightButtons: [],
leftButtons: []
}
});

changeTitle = () => Navigation.mergeOptions(this, {
topBar: {
title: {
Expand Down
1 change: 1 addition & 0 deletions playground/src/screens/Screens.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ module.exports = {
Navigation: 'Navigation',
NativeScreen: 'RNNCustomComponent',
RoundButton: 'CustomRoundedButton',
LifecycleButton: 'LifecycleButton',
ReactTitleView: 'ReactTitleView',
EventsScreen: 'EventsScreen',
EventsOverlay: 'EventsOverlay',
Expand Down
6 changes: 6 additions & 0 deletions playground/src/screens/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ function registerScreens() {
Navigation.registerComponent(Screens.Lifecycle, () => require('./LifecycleScreen'));
Navigation.registerComponent(Screens.Overlay, () => require('./OverlayScreen'));
Navigation.registerComponent(Screens.OverlayAlert, () => require('./OverlayAlert'));
Navigation.registerComponent(Screens.Pushed, () => require('./PushedScreen'));
Navigation.registerComponent(Screens.ScrollViewOverlay, () => require('./ScrollViewOverlay'));
Navigation.registerComponent(Screens.RoundButton, () => require('./RoundedButton'));
Navigation.registerComponent(Screens.LifecycleButton, () => require('./LifecycleButton'));
Navigation.registerComponent(Screens.ReactTitleView, () => require('./CustomTopBar'));
Navigation.registerComponent(Screens.RoundButton, () => require('./RoundedButton'));
Navigation.registerComponent(Screens.ScrollViewOverlay, () => require('./ScrollViewOverlay'));
Navigation.registerComponent(Screens.RoundButton, () => require('./RoundedButton'));
Navigation.registerComponent(Screens.ReactTitleView, () => require('./CustomTopBar'));
Expand Down
2 changes: 2 additions & 0 deletions playground/src/testIDs.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ module.exports = {
PUSH_BOTTOM_TABS_BUTTON: `PUSH_BOTTOM_TABS_BUTTON`,
SET_STACK_ROOT_BUTTON: `SET_STACK_ROOT_BUTTON`,
SET_ROOT:'SET_ROOT',
RESET_BUTTONS: 'RESET_BUTTONS',
SHOW_LIFECYCLE_BTN: 'SHOW_LIFECYCLE_BTN',

// Elements
SCROLLVIEW_ELEMENT: `SCROLLVIEW_ELEMENT`,
Expand Down

0 comments on commit 65dde34

Please sign in to comment.