Discussion only - related to #4124 #4127
Closed
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
Following commits will add support for handling receving sharing from other apps. `NotifyReact.kt` contains useful functions that can be reused, so refactor it to make them available outside it.
Makes the fields subject,localId, and eventQueueId optional. These are not always required to be sent to the send-message endpoint, and this modified form will be used in the sharing module.
Formats the file 'android/app/src/main/AndroidManifest.xml' according to Default Android Studio recommended formatting.
Enables handling and receiving shares from other apps, for any type of content (text, images, and other files). Creates a native RN module `SharingModule` to handle the Android part of this feature. 'ReceiveShareActivity.kt' parses the received shared data and handles activating the JS part of the codebase - either by sending an event to an already-running app, or launching `MainActivity` with some initial shared data via the native module. Registers a new root component, `SharingRoot`. It is a dummy component that does nothing, and `ReceiveShareActivity.kt`. It exists because 'Sharing' is linked with launching an activity in the Android ecosystem. But we don't always want to launch `MainActivity` when receiving a share because it may cause two instances of it to be opened simultaneously. We can't check whether the app is running before an activity launches. So, we launch this dummy component, then process the events, identify whether the app is running or not, and handle that as mentioned in the paragraph above, and then quickly kill this dummy Activity. All of this happens fast enough that the component does not even get time to render, so it's seamless. Closes #M117.
Something with "Navigator" makes more sense to me, as the output of `create*Navigator`.
This is just to minimize the diff in the next commit, so it's easier to follow along there.
…eToPm This is just meant to show one way this could be done -- it seems more React- and React-Navigation idiomatic. It avoids calling `createMaterialTopTabNavigator` from within a render method or a constructor. This uses the alternative mentioned at the "Common Mistakes" doc [1], with the `router` static and threading through the `navigation` prop. There are a few comments I would make about some of the code, not addressed in this commit, but hopefully this will help for conceptual clarity about the kind of thing I'm suggesting. :) More discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60screenProps.60.20in.20react-navigation.20v2/near/888521. [1]: https://reactnavigation.org/docs/2.x/common-mistakes/#explicitly-rendering-more-than-one-navigator
This better satisfies React Navigation's "Common Mistakes" thing about only explicitly rendering one navigator. Looks like the UI breaks, though. Hmm.
See that the log of `sharedData`, in `ShareToStreamComponent`, shows what you'd expect it to. I also had to disable some `getAuth`s, as I was getting the "Active account not logged in" error.
|
Thanks a lot @chrisbobbe! 😄 |
|
I think this PR has served its purpose. Closing! 🙂 |
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Oct 1, 2020
…prop. This has been done on an as-needed basis in the past, but in a few different ways, and it would be good to be consistent. With zulip#3804 approaching, it makes sense to proactively furnish all these components with the knowledge that they have the `navigation` prop if they ever need it. In doing so, we're making an unchecked assumption: that the components will in fact be passed the `navigation` prop, in the shape we say it will take. It's unchecked because of the `$FlowFixMe` from aaaf847 on the route config passed to `createStackNavigator` [0]. ----- There is a doc for doing this in TypeScript [1], but it's clearly wrong about how to handle it in Flow (and I believe it's also wrong about TypeScript in ways that would matter to us if we were using TypeScript). In particular, `NavigationStackScreenProps` is marketed (under "Type checking all props for a screen") as something you can spread in your `Props` type to get `theme` and `screenProps` along with `navigation`. Sounds great. But it doesn't exist in the libdef, and I haven't found a clear alternative. In zulip#4127, a demo PR branch that informed 17f73d8 -- in particular, f482827 [FOR DISCUSSION] One way to get `sharedData` into ShareToStream, ShareToPm -- I seemed [2] to have favored something called `NavigationNavigatorProps` for this purpose. But it has at least two points against it: 1. I don't see documentation that we're supposed to use it, or how. 2. It's not tailored (or conveniently tailorable) to screens that get put on a particular type of navigator (stack, drawer, tabs, etc.) So, take a conservative approach and just type the `navigation` prop, and in a way that says we know it comes from a stack navigator. The main example in the TypeScript doc is the following: ``` type Props = { navigation: NavigationStackProp<{ userId: string }>; }; ``` We find `NavigationStackProp` is a good thing to use, but `{ userId: string }` is bad for a couple of reasons: - It's off by a level of nesting; surely they mean to describe `navigation.state.params`. To do that, the type needs to look something like `{ params: { userId: string } }`. - It leaves out all the other things on `navigation.state` including `key` and `routeName`, which might be nice to have someday. Experimentally, these are conveniently filled in by saying `NavigationStackProp<NavigationStateRoute>` [3]. So, account for those deficiencies straightforwardly. [0] Initial thoughts on this were written in a36814e, but see zulip#4114 (comment) for more recent concerns about typing our component classes with a static `navigationOptions` property. I'm inclined to not revisit these suppressions until we're on React Navigation v5, which has quite a different, component-based API, and a particular guide for type-checking the route config and the screen components in a unified way; that's at https://reactnavigation.org/docs/typescript/. [1] https://reactnavigation.org/docs/4.x/typescript/ [2] Possibly following a train of thought from https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878262. [3] Like at react-navigation/react-navigation#3643 (comment), but with `NavigationStackProp` instead of `NavigationScreenProp`.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Oct 1, 2020
…prop. This has been done on an as-needed basis in the past, but in a few different ways, and it would be good to be consistent. With zulip#3804 approaching, it makes sense to proactively furnish all these components with the knowledge that they have the `navigation` prop if they ever need it. In doing so, we're making an unchecked assumption: that the components will in fact be passed the `navigation` prop, in the shape we say it will take. It's unchecked because of the `$FlowFixMe` from aaaf847 on the route config passed to `createStackNavigator` [0]. ----- There is a doc for doing this in TypeScript [1], but it's clearly wrong about how to handle it in Flow (and I believe it's also wrong about TypeScript in ways that would matter to us if we were using TypeScript). In particular, `NavigationStackScreenProps` is marketed (under "Type checking all props for a screen") as something you can spread in your `Props` type to get `theme` and `screenProps` along with `navigation`. Sounds great. But it doesn't exist in the libdef, and I haven't found a clear alternative. In zulip#4127, a demo PR branch that informed 17f73d8 -- in particular, f482827 [FOR DISCUSSION] One way to get `sharedData` into ShareToStream, ShareToPm -- I seemed [2] to have favored something called `NavigationNavigatorProps` for this purpose. But it has at least two points against it: 1. I don't see documentation that we're supposed to use it, or how. 2. It's not tailored (or conveniently tailorable) to screens that get put on a particular type of navigator (stack, drawer, tabs, etc.) So, take a conservative approach and just type the `navigation` prop, and in a way that says we know it comes from a stack navigator. The main example in the TypeScript doc is the following: ``` type Props = { navigation: NavigationStackProp<{ userId: string }>; }; ``` We find `NavigationStackProp` is a good thing to use, but `{ userId: string }` is bad for a couple of reasons: - It's off by a level of nesting; surely they mean to describe `navigation.state.params`. To do that, the type needs to look something like `{ params: { userId: string } }`. - It leaves out all the other things on `navigation.state` including `key` and `routeName`, which might be nice to have someday. Experimentally, these are conveniently filled in by saying `NavigationStackProp<NavigationStateRoute>` [3]. So, account for those deficiencies straightforwardly. [0] Initial thoughts on this were written in a36814e, but see zulip#4114 (comment) for more recent concerns about typing our component classes with a static `navigationOptions` property. I'm inclined to not revisit these suppressions until we're on React Navigation v5, which has quite a different, component-based API, and a particular guide for type-checking the route config and the screen components in a unified way; that's at https://reactnavigation.org/docs/typescript/. [1] https://reactnavigation.org/docs/4.x/typescript/ [2] Possibly following a train of thought from https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878262. [3] Like at react-navigation/react-navigation#3643 (comment), but with `NavigationStackProp` instead of `NavigationScreenProp`.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Oct 2, 2020
…prop. This has been done on an as-needed basis in the past, but in a few different ways, and it would be good to be consistent. With zulip#3804 approaching, it makes sense to proactively furnish all these components with the knowledge that they have the `navigation` prop if they ever need it. In doing so, we're making an unchecked assumption: that the components will in fact be passed the `navigation` prop, in the shape we say it will take. It's unchecked because of the `$FlowFixMe` from aaaf847 on the route config passed to `createStackNavigator` [0]. ----- There is a doc for doing this in TypeScript [1], but it's clearly wrong about how to handle it in Flow (and I believe it's also wrong about TypeScript in ways that would matter to us if we were using TypeScript). In particular, `NavigationStackScreenProps` is marketed (under "Type checking all props for a screen") as something you can spread in your `Props` type to get `theme` and `screenProps` along with `navigation`. Sounds great. But it doesn't exist in the Flow libdef, and I haven't found a clear alternative. In zulip#4127, a demo PR branch that informed 17f73d8 -- in particular, f482827 [FOR DISCUSSION] One way to get `sharedData` into ShareToStream, ShareToPm -- I seemed [2] to have favored something called `NavigationNavigatorProps` for this purpose. But it has at least two points against it: 1. I don't see documentation that we're supposed to use it, or how. 2. It's not tailored (or conveniently tailorable) to screens that get put on a particular type of navigator (stack, drawer, tabs, etc.) So, be conservative and just type the `navigation` prop, and in a way that says we know it comes from a stack navigator. The main example in the TypeScript doc is the following: ``` type Props = { navigation: NavigationStackProp<{ userId: string }>; }; ``` We find `NavigationStackProp` is a good thing to use [3], and it exists in the Flow libdef, but `{ userId: string }` is bad for a couple of reasons: - It's off by a level of nesting; surely they mean to describe `navigation.state.params`. To do that, the type needs to look something like `{ params: { userId: string } }`. This is also the case in the TypeScript. - It leaves out all the other things on `navigation.state` including `key` and `routeName`, which might be nice to have someday. Experimentally, these are conveniently filled in by saying `NavigationStackProp<NavigationStateRoute>` [4]. So, account for those deficiencies straightforwardly. [0] Initial thoughts on this were written in a36814e, but see zulip#4114 (comment) for more recent concerns about typing our component classes with a static `navigationOptions` property. I'm inclined to not revisit these suppressions until we're on React Navigation v5, which has quite a different, component-based API, and a particular guide for type-checking the route config and the screen components in a unified way; that's at https://reactnavigation.org/docs/typescript/. [1] https://reactnavigation.org/docs/4.x/typescript/ [2] Possibly following a train of thought from https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878262. [3] In one or two places, we've been using `NavigationScreenProp` -- the doc for upgrading from v3 (which we did in f3b6c1f) says to replace that with `NavigationStackProp` for stack navigators. This didn't catch my eye at the time because it's under the "TypeScript" heading and we don't use TypeScript. That doc is at https://reactnavigation.org/docs/4.x/upgrading-from-3.x/#typescript. [4] Like at react-navigation/react-navigation#3643 (comment), but with `NavigationStackProp` instead of `NavigationScreenProp`.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Oct 3, 2020
…prop. This has been done on an as-needed basis in the past, but in a few different ways, and it would be good to be consistent. With zulip#3804 approaching, it makes sense to proactively furnish all these components with the knowledge that they have the `navigation` prop if they ever need it. In doing so, we're making an unchecked assumption: that the components will in fact be passed the `navigation` prop, in the shape we say it will take. It's unchecked because of the `$FlowFixMe` from aaaf847 on the route config passed to `createStackNavigator` [0]. ----- There is a doc for doing this in TypeScript [1], but it's clearly wrong about how to handle it in Flow (and I believe it's also wrong about TypeScript in ways that would matter to us if we were using TypeScript). In particular, `NavigationStackScreenProps` is marketed (under "Type checking all props for a screen") as something you can spread in your `Props` type to get `theme` and `screenProps` along with `navigation`. Sounds great. But it doesn't exist in the Flow libdef, and I haven't found a clear alternative. In zulip#4127, a demo PR branch that informed 17f73d8 -- in particular, f482827 [FOR DISCUSSION] One way to get `sharedData` into ShareToStream, ShareToPm -- I seemed [2] to have favored something called `NavigationNavigatorProps` for this purpose. But it has at least two points against it: 1. I don't see documentation that we're supposed to use it, or how. 2. It's not tailored (or conveniently tailorable) to screens that get put on a particular type of navigator (stack, drawer, tabs, etc.) So, be conservative and just type the `navigation` prop, and in a way that says we know it comes from a stack navigator. The main example in the TypeScript doc is the following: ``` type Props = { navigation: NavigationStackProp<{ userId: string }>; }; ``` We find `NavigationStackProp` is a good thing to use [3], and it exists in the Flow libdef, but `{ userId: string }` is bad for a couple of reasons: - It's off by a level of nesting; surely they mean to describe `navigation.state.params`. To do that, the type needs to look something like `{ params: { userId: string } }`. This is also the case in the TypeScript. - It leaves out all the other things on `navigation.state` including `key` and `routeName`, which might be nice to have someday. Experimentally, these are conveniently filled in by saying `NavigationStackProp<NavigationStateRoute>` [4]. So, account for those deficiencies straightforwardly. [0] Initial thoughts on this were written in a36814e, but see zulip#4114 (comment) for more recent concerns about typing our component classes with a static `navigationOptions` property. I'm inclined to not revisit these suppressions until we're on React Navigation v5, which has quite a different, component-based API, and a particular guide for type-checking the route config and the screen components in a unified way; that's at https://reactnavigation.org/docs/typescript/. [1] https://reactnavigation.org/docs/4.x/typescript/ [2] Possibly following a train of thought from https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878262. [3] In one or two places, we've been using `NavigationScreenProp` -- the doc for upgrading from v3 (which we did in f3b6c1f) says to replace that with `NavigationStackProp` for stack navigators. This didn't catch my eye at the time because it's under the "TypeScript" heading and we don't use TypeScript. That doc is at https://reactnavigation.org/docs/4.x/upgrading-from-3.x/#typescript. [4] Like at react-navigation/react-navigation#3643 (comment), but with `NavigationStackProp` instead of `NavigationScreenProp`.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Oct 5, 2020
…prop. This has been done on an as-needed basis in the past, but in a few different ways, and it would be good to be consistent. With zulip#3804 approaching, it makes sense to proactively furnish all these components with the knowledge that they have the `navigation` prop if they ever need it. In doing so, we're making an unchecked assumption: that the components will in fact be passed the `navigation` prop, in the shape we say it will take. It's unchecked because of the `$FlowFixMe` from aaaf847 on the route config passed to `createStackNavigator` [0]. ----- There is a doc for doing this in TypeScript [1], but it's clearly wrong about how to handle it in Flow (and I believe it's also wrong about TypeScript in ways that would matter to us if we were using TypeScript). In particular, `NavigationStackScreenProps` is marketed (under "Type checking all props for a screen") as something you can spread in your `Props` type to get `theme` and `screenProps` along with `navigation`. Sounds great. But it doesn't exist in the Flow libdef, and I haven't found a clear alternative. In zulip#4127, a demo PR branch that informed 17f73d8 -- in particular, f482827 [FOR DISCUSSION] One way to get `sharedData` into ShareToStream, ShareToPm -- I seemed [2] to have favored something called `NavigationNavigatorProps` for this purpose. But it has at least two points against it: 1. I don't see documentation that we're supposed to use it, or how. 2. It's not tailored (or conveniently tailorable) to screens that get put on a particular type of navigator (stack, drawer, tabs, etc.) So, be conservative and just type the `navigation` prop, and in a way that says we know it comes from a stack navigator. The main example in the TypeScript doc is the following: ``` type Props = { navigation: NavigationStackProp<{ userId: string }>; }; ``` We find `NavigationStackProp` is a good thing to use [3], and it exists in the Flow libdef, but `{ userId: string }` is bad for a couple of reasons: - It's off by a level of nesting; surely they mean to describe `navigation.state.params`. To do that, the type needs to look something like `{ params: { userId: string } }`. This is also the case in the TypeScript. - It leaves out all the other things on `navigation.state` including `key` and `routeName`, which might be nice to have someday. Experimentally, these are conveniently filled in by saying `NavigationStackProp<NavigationStateRoute>` [4]. So, account for those deficiencies straightforwardly. [0] Initial thoughts on this were written in a36814e, but see zulip#4114 (comment) for more recent concerns about typing our component classes with a static `navigationOptions` property. I'm inclined to not revisit these suppressions until we're on React Navigation v5, which has quite a different, component-based API, and a particular guide for type-checking the route config and the screen components in a unified way; that's at https://reactnavigation.org/docs/typescript/. [1] https://reactnavigation.org/docs/4.x/typescript/ [2] Possibly following a train of thought from https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878262. [3] In one or two places, we've been using `NavigationScreenProp` -- the doc for upgrading from v3 (which we did in f3b6c1f) says to replace that with `NavigationStackProp` for stack navigators. This didn't catch my eye at the time because it's under the "TypeScript" heading and we don't use TypeScript. That doc is at https://reactnavigation.org/docs/4.x/upgrading-from-3.x/#typescript. [4] Like at react-navigation/react-navigation#3643 (comment), but with `NavigationStackProp` instead of `NavigationScreenProp`.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Oct 5, 2020
…prop. This has been done on an as-needed basis in the past, but in a few different ways, and it would be good to be consistent. With zulip#3804 approaching, it makes sense to proactively furnish all these components with the knowledge that they have the `navigation` prop if they ever need it. In doing so, we're making an unchecked assumption: that the components will in fact be passed the `navigation` prop, in the shape we say it will take. It's unchecked because of the `$FlowFixMe` from aaaf847 on the route config passed to `createStackNavigator` [0]. ----- There is a doc for doing this in TypeScript [1], but it's clearly wrong about how to handle it in Flow (and I believe it's also wrong about TypeScript in ways that would matter to us if we were using TypeScript). In particular, `NavigationStackScreenProps` is marketed (under "Type checking all props for a screen") as something you can spread in your `Props` type to get `theme` and `screenProps` along with `navigation`. Sounds great. But it doesn't exist in the Flow libdef, and I haven't found a clear alternative. In zulip#4127, a demo PR branch that informed 17f73d8 -- in particular, f482827 [FOR DISCUSSION] One way to get `sharedData` into ShareToStream, ShareToPm -- I seemed [2] to have favored something called `NavigationNavigatorProps` for this purpose. But it has at least two points against it: 1. I don't see documentation that we're supposed to use it, or how. 2. It's not tailored (or conveniently tailorable) to screens that get put on a particular type of navigator (stack, drawer, tabs, etc.) So, be conservative and just type the `navigation` prop, and in a way that says we know it comes from a stack navigator. The main example in the TypeScript doc is the following: ``` type Props = { navigation: NavigationStackProp<{ userId: string }>; }; ``` We find `NavigationStackProp` is a good thing to use [3], and it exists in the Flow libdef, but `{ userId: string }` is bad for a couple of reasons: - It's off by a level of nesting; surely they mean to describe `navigation.state.params`. To do that, the type needs to look something like `{ params: { userId: string } }`. This is also the case in the TypeScript. - It leaves out all the other things on `navigation.state` including `key` and `routeName`, which might be nice to have someday. Experimentally, these are conveniently filled in by saying `NavigationStackProp<NavigationStateRoute>` [4]. So, account for those deficiencies straightforwardly. [0] Initial thoughts on this were written in a36814e, but see zulip#4114 (comment) for more recent concerns about typing our component classes with a static `navigationOptions` property. I'm inclined to not revisit these suppressions until we're on React Navigation v5, which has quite a different, component-based API, and a particular guide for type-checking the route config and the screen components in a unified way; that's at https://reactnavigation.org/docs/typescript/. [1] https://reactnavigation.org/docs/4.x/typescript/ [2] Possibly following a train of thought from https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878262. [3] In one or two places, we've been using `NavigationScreenProp` -- the doc for upgrading from v3 (which we did in f3b6c1f) says to replace that with `NavigationStackProp` for stack navigators. This didn't catch my eye at the time because it's under the "TypeScript" heading and we don't use TypeScript. That doc is at https://reactnavigation.org/docs/4.x/upgrading-from-3.x/#typescript. [4] Like at react-navigation/react-navigation#3643 (comment), but with `NavigationStackProp` instead of `NavigationScreenProp`.
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.
An example, to illustrate my ideas at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60screenProps.60.20in.20react-navigation.20v2/near/888521.