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

Typo fix in 'docs-basic-navigation.mdx' #6214

Closed
wants to merge 4 commits into from

Conversation

andrius-girdzius
Copy link

@andrius-girdzius andrius-girdzius commented May 13, 2020

  1. Code snippets in the documentation file had the style value incorrectly set as 'style.home' rather than 'style.root' which is defined in the file.
const HomeScreen = (props) => {
  return (
    <View style={styles.home}>
      <Text>Home Screen</Text>
    </View>
  );
};

...

const styles = StyleSheet.create({
  root: {
    flex: 1,
    alignItems: 'center',
    justifyContent: 'center',
    backgroundColor: 'whitesmoke'
  }
});
  1. SettingsScreen declared twice in two code snippets.
// Settings screen declaration - this is the screen we'll be pushing into the stack
const SettingsScreen = () => {
  return (
    <View style={[styles.root, styles.settings]}>
      <Text>Settings Screen</Text>
    </View>
  );
};

...

const SettingsScreen = () => {
  return (
    <View style={styles.root}>
      <Text>Settings Screen</Text>
    </View>
  );
}
  1. Finally, the app seems to crash on iOS (https://imgur.com/a/gcroTSX) possibly due to .setDefaultOptions() being called too early. The following change seems to work:

BEFORE:

Navigation.setDefaultOptions({
  statusBar: {
    backgroundColor: '#4d089a'
  },
  topBar: {
    title: {
      color: 'white'
    },
    backButton: {
      color: 'white'
    },
    background: {
      color: '#4d089a'
    }
  }
});
Navigation.events().registerAppLaunchedListener(async () => {
  Navigation.setRoot({
    root: {
      stack: {
        children: [
          {
            component: {
              name: 'Home'
            }
          }
        ]
      }
    }
  });
});

AFTER:

Navigation.events().registerAppLaunchedListener(async () => {
  Navigation.setDefaultOptions({
    statusBar: {
      backgroundColor: '#4d089a'
    },
    topBar: {
      title: {
        color: 'white'
      },
      backButton: {
        color: 'white'
      },
      background: {
        color: '#4d089a'
      }
    }
  });
  Navigation.setRoot({
    root: {
      stack: {
        children: [
          {
            component: {
              name: 'Home'
            }
          }
        ]
      }
    }
  });
});

@andrius-girdzius andrius-girdzius marked this pull request as ready for review May 13, 2020 13:55
…e .onAppLaunched(). Removed old duplicate SettingsScreen declaration.
…ed before .onAppLaunched(). Removed old duplicate SettingsScreen declaration."

This reverts commit 7f794b3
…ed before .onAppLaunched(). Removed old duplicate SettingsScreen declaration."
@andrius-girdzius andrius-girdzius changed the title Style value typo fix in 'docs-basic-navigation.mdx' Typo fix in 'docs-basic-navigation.mdx' May 13, 2020
@andrius-girdzius
Copy link
Author

Added more fixes for the page and updated the request details.

@andrius-girdzius andrius-girdzius linked an issue May 13, 2020 that may be closed by this pull request
},
backButton: {
color: 'white'
Navigation.events().registerAppLaunchedListener(async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@andrius-wix Why did you move the call to setDefaultOptions inside of the app launched listener? This will still work, but not obligatory. default options can be set before the app launched event is received.

Copy link
Author

Choose a reason for hiding this comment

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

Running the entire code snippet unchanged (with the exception of removing the second SettingsScreen declaration) throws the exception below on iOS 13.2.2 (simulator). Seems like other users have had a similar error in the past as well (see #5104 for an example, more on Google).

Could you please verify if there is anything else I am missing? RNN 6.6.0.

Exception 'Bridge not yet loaded! Send commands after Navigation.events().onAppLaunched() has been called.' was thrown while invoking setDefaultOptions on target RNNBridgeModule with params (
        {
        statusBar =         {
            backgroundColor = 4283238554;
        };
        topBar =         {
            backButton =             {
                color = 4294967295;
            };
            background =             {
                color = 4283238554;
            };
            title =             {
                color = 4294967295;
            };
        };
    },
    34,
    35
)
callstack: (
	0   CoreFoundation                      0x000000010a03502e __exceptionPreprocess + 350
	1   libobjc.A.dylib                     0x0000000109eadb20 objc_exception_throw + 48
	2   CoreFoundation                      0x000000010a034bf9 -[NSException raise] + 9
	3   CoviData                            0x0000000105b54748 -[RNNCommandsHandler assertReady] + 136
	4   CoviData                            0x0000000105b4e2dd -[RNNCommandsHandler setDefaultOptions:completion:] + 93
	5   CoviData                            0x0000000105b49d32 __55-[RNNBridgeModule setDefaultOptions:resolver:rejecter:]_block_invoke + 146
	6   CoviData                            0x0000000105918293 RCTExecuteOnMainQueue + 67
	7   CoviData                            0x0000000105b49c1b -[RNNBridgeModule setDefaultOptions:resolver:rejecter:] + 251
	8   CoreFoundation                      0x000000010a03c1cc __invoking___ + 140
	9   CoreFoundation                      0x000000010a0392df -[NSInvocation invoke] + 319
	10  CoreFoundation                      0x000000010a0397e4 -[NSInvocation invokeWithTarget:] + 68
	11  CoviData                            0x00000001058a2de2 -[RCTModuleMethod invokeWithBridge:module:arguments:] + 2658
	12  CoviData                            0x00000001058a6f17 _ZN8facebook5reactL11invokeInnerEP9RCTBridgeP13RCTModuleDatajRKN5folly7dynamicE + 791
	13  CoviData                            0x00000001058a6a23 _ZZN8facebook5react15RCTNativeModule6invokeEjON5folly7dynamicEiENK3$_0clEv + 131
	14  CoviData                            0x00000001058a6999 ___ZN8facebook5react15RCTNativeModule6invokeEjON5folly7dynamicEi_block_invoke + 25
	15  libdispatch.dylib                   0x000000010cfe6848 _dispatch_call_block_and_release + 12
	16  libdispatch.dylib                   0x000000010cfe77b9 _dispatch_client_callout + 8
	17  libdispatch.dylib                   0x000000010cff3c9b _dispatch_main_queue_callback_4CF + 1212
	18  CoreFoundation                      0x0000000109f97df9 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
	19  CoreFoundation                      0x0000000109f92a59 __CFRunLoopRun + 2329
	20  CoreFoundation                      0x0000000109f91e16 CFRunLoopRunSpecific + 438
	21  GraphicsServices                    0x000000010e4e9bb0 GSEventRunModal + 65
	22  UIKitCore                           0x0000000113687b48 UIApplicationMain + 1621
	23  CoviData                            0x00000001052c65e0 main + 112
	24  libdyld.dylib                       0x000000010d068c25 start + 1
	25  ???                                 0x0000000000000001 0x0 + 1
)

RCTFatal
facebook::react::invokeInner(RCTBridge*, RCTModuleData*, unsigned int, folly::dynamic const&)
facebook::react::RCTNativeModule::invoke(unsigned int, folly::dynamic&&, int)::$_0::operator()() const
invocation function for block in facebook::react::RCTNativeModule::invoke(unsigned int, folly::dynamic&&, int)
_dispatch_call_block_and_release
_dispatch_client_callout
_dispatch_main_queue_callback_4CF
__CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__
__CFRunLoopRun
CFRunLoopRunSpecific
GSEventRunModal
UIApplicationMain
main
start
0x0

guyca added a commit that referenced this pull request May 18, 2020
Support for the react-native-youtube package was added in #5999 but due to a faulty merge it wasn't complete.

Fixes #6214
@guyca guyca closed this in #6220 May 18, 2020
guyca added a commit that referenced this pull request May 18, 2020
Support for the react-native-youtube package was added in #5999 but due to a faulty merge it wasn't complete.

Fixes #6214
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example code in the 'Basic Navigation' is faulty
2 participants