Move BackAndroid -> BackHandler, add Apple TV support for back nav #12571
Conversation
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
@satya164 @ericvicenti This is a better implementation of back navigation for Apple TV. One feature still missing: when the user is on the bottom of the nav stack, and presses the menu button, Apple TV guidelines require the app to go to background and the tvOS home screen to show. That will require some native changes and a separate PR. |
|
Overall, this seems like a great direction to move in. This is a platform-agnostic concept. The test plan doesn't mention testing on non-tv platforms, so we'd need to see that tested. Can we warn people when they use Also the documentation doesn't seem to be affected, but we need to document this change very carefully. |
|
The module is just a stub on iOS - the new functionality is only on tvOS. The Android module is unchanged. I'd have to think about how we would detect and generate a warning when an application imports BackAndroid instead of BackHandler. Agreed on the documentation. |
|
For the warning, we can have a BackAndroid as a wrapper module around BackHandler, that warns whenever anything on it is called. |
| @@ -34,20 +34,29 @@ RCTDeviceEventEmitter.addListener(DEVICE_BACK_EVENT, function() { | |||
| } | |||
There was a problem hiding this comment.
no-trailing-spaces: Trailing spaces not allowed.
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| * | ||
| * On Apple TV, this implements back navigation using the TV remote's menu button. | ||
| * On iOS, this just implements a stub. |
There was a problem hiding this comment.
no-trailing-spaces: Trailing spaces not allowed.
|
|
||
| 'use strict'; | ||
|
|
||
| var Platform = require('Platform'); |
| 'use strict'; | ||
|
|
||
| var Platform = require('Platform'); | ||
| var TVEventHandler = require('TVEventHandler'); |
| var Platform = require('Platform'); | ||
| var TVEventHandler = require('TVEventHandler'); | ||
|
|
||
| type BackPressEventName = $Enum<{ |
There was a problem hiding this comment.
Wondering why it's not type BackPressEventName = 'backPress' instead
Also seems all examples say that the event name is hardwareBackPress, while the type says backPress
There was a problem hiding this comment.
That's copied from the Android version.
| * }); | ||
| * ``` | ||
| */ | ||
| var BackHandler; |
| * return false; | ||
| * }); | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
nit: new line after comment
| remove: () => BackHandler.removeEventListener(eventName, handler), | ||
| }; | ||
| }, | ||
|
|
There was a problem hiding this comment.
nit: unnecessary new line
| ): void { | ||
| _backPressSubscriptions.delete(handler); | ||
| }, | ||
|
|
There was a problem hiding this comment.
nit: unnecessary new line
| }, | ||
|
|
||
| }; | ||
|
|
There was a problem hiding this comment.
nit: unnecessary new lines
|
|
||
|
|
||
| } else { | ||
|
|
There was a problem hiding this comment.
nit: unnecessary new lines
| }, | ||
| removeEventListener: emptyFunction, | ||
| }; | ||
|
|
There was a problem hiding this comment.
nit: unnecessary new lines
…n for tvOS without this change)
|
@sahrens Correct, RefreshControl doesn't have native support on tvOS. Looks like the least intrusive solution is to do the platform check in ScrollView (which has a lot of platform-specific code already). This automatically takes care of all the other components like VirtualizedList that make use of scroll views. |
|
FYI, this will also be a great thing for |
| '../Libraries/Utilities/BackAndroid.android.js', | ||
| '../Libraries/Utilities/BackAndroid.js', | ||
| '../Libraries/Utilities/BackHandler.ios.js', | ||
| '../Libraries/Utilities/BackHandler.android.js', |
There was a problem hiding this comment.
Does this cause there to be 3 separate pages? Lets consolidate it so that all the BackHandler docs are on one page.
There was a problem hiding this comment.
There were only two pages when I tested this, one for BackHandler, and one for BackAndroid to let users know that it is deprecated.
|
Also I think the RefreshControl change should be done separately |
|
Ok I'll back out the RefreshControl change and submit as a separate PR |
facebook-github-bot
added
GH Review: accepted
Import Started
|
@ericvicenti has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Enable back navigation on Apple TV (with the remote's menu button) in code making use of BackAndroid. The module is renamed to BackHandler. BackAndroid is still exported to ReactNative for now, until external projects switch to using the new name for the module. The navigation in https://github.com/react-community/react-navigation makes use of this module. **Test plan**: Manual testing with an example app (https://github.com/dlowder-salesforce/react-nav-example). Closes facebook/react-native#12571 Differential Revision: D4665152 Pulled By: ericvicenti fbshipit-source-id: 925400ce216379267e014457be6f5eedbe4453ec
Enable back navigation on Apple TV (with the remote's menu button) in code making use of BackAndroid. The module is renamed to BackHandler. BackAndroid is still exported to ReactNative for now, until external projects switch to using the new name for the module. The navigation in https://github.com/react-community/react-navigation makes use of this module.
Test plan: Manual testing with an example app (https://github.com/dlowder-salesforce/react-nav-example).