[Navigator] add optional callbacks to routing functions #1308

pwmckenna · 2015-05-16T01:21:11Z

No description provided.

ericvicenti · 2015-05-19T17:23:54Z

This might be confusing because the callbacks will resolve before the animation completes in the case of push, but it will call back after the animation for pop, etc.

Could you use the willFocus/didFocus API for this?

pwmckenna · 2015-05-20T17:40:01Z

is the reason that it resolves before the animation completes for push because _transitionTo kicks something off (and doesn't accept a callback)? I could try to fix that. What the callbacks are good for (and where willFocus/didFocus would struggle i believe) is doing something like popping, waiting for that to finish, then pushing something else on. I could certainly write a wrapper around willFocus/didFocus that ties them to the most recent navigation and provides the same type of callback, but it would be much more complicated.

ericvicenti · 2015-05-20T18:04:13Z

Ok, I see what you mean. This API does sound useful, even though we haven't needed it internally yet.

If you look at _transitionTo, it actually does accept a callback. You could pass it through and make it call back after the push transition.

pwmckenna · 2015-05-20T18:13:40Z

fantastic! i'll patch it up with the transitionTo callback. I have no idea how to write tests for this. suggestions?

ericvicenti · 2015-05-20T18:26:16Z

Right now we're totally lacking tests for navigator, partially because it can be tricky to mock out the animations. I'm going to add some tests in the next week or two, and I can also add a few tests for the callbacks.

ide · 2015-05-20T19:38:06Z

I think we want willLoseFocus and didLoseFocus lifecycle events, or something of that nature. Worth looking at UIViewController's transition API (it's complex...) and Android fragments to get a sense of what events a scene might want to receive and when those events should be fired.

pwmckenna · 2015-05-20T21:33:58Z

@ericvicenti made the push change, and added cb arguments to the additional exposed methods that I missed initially. What is the process for getting things merged in for this project? Should I squash commits? Wait for the tests (which I'd be more than happy to help with btw)? Happy to help however.

@ide Those lifecycle hooks sound super useful. I'd be happy to help with those as well, though I don't have any experience with UIViewController or Android fragments, and I'm not super in tune with what the long term goals of this project are as they pertain to android.

ericvicenti · 2015-05-21T00:01:28Z

@facebook-github-bot import

I'll test this manually for now and get it merged this week. Meanwhile, if you want to get going on the tests, I can point you in the right direction tomorrow.

ericvicenti · 2015-05-26T16:16:32Z

Libraries/CustomComponents/Navigator/Navigator.js

    var destIndex = this._getDestIndexWithinBounds(n);
    var requestTransitionAndResetUpdatingRange = () => {
      this._enableScene(destIndex);
-      this._transitionTo(destIndex);
+      this._transitionTo(destIndex, cb);


cb is the fourth argument in _transitionTo, so I don't think this will work

good call. updated.

ericvicenti · 2015-06-08T23:20:38Z

Sorry for the delay here. I've been talking with @ide and have had a change of heart: instead of having callbacks on all of these actions, we could add focus lifecycle events that get called on the scenes:

willFocus
didFocus
willLooseFocus
didLooseFocus

I think these lifecycle methods will be more versatile than callbacks on the actions. Thoughts?

ide · 2015-06-08T23:38:45Z

Here are the specifics as a strawman:

willFocus: when the navigator has committed to displaying a scene. For example, if you pan from the edge of the screen far enough to dismiss the topmost scene, the navigator will fire willFocus for the scene below when you lift your finger because the navigator has committed to dismissing the top scene. If you start panning but then don't pan far enough, the navigator does not fire the willFocus event because it will not focus the scene below (sounds obvious... just stating it clearly).
didFocus: when the navigator has finished completely displaying a scene. If there is a transition animation after willFocus, then the navigator fires didFocus after the transition completes. If there is no transition, then the navigator simply fires didFocus after willFocus.
willBlur: fired for the dismissed scene right before the navigator fires willFocus for the scene it will transition to
didBlur: fired for the dismissed scene right before the navigator fires didFocus for the scene that it did transition to

Add comma and space

Summary: **Context** On Core RN, Hermes for iOS can be enabled by setting a flag in the Podfile https://reactnative.dev/docs/hermes#ios | Since React Native 0.64, Hermes also runs on iOS. To enable Hermes for iOS, edit your ios/Podfile file and make the change illustrated below: ``` use_react_native!( :path => config[:reactNativePath], # to enable hermes on iOS, change `false` to `true` and then install pods # By default, Hermes is disabled on Old Architecture, and enabled on New Architecture. # You can enable/disable it manually by replacing `flags[:hermes_enabled]` with `true` or `false`. :hermes_enabled => true ) ``` In the RNTester Podfile, Hermes is enabled using envvar: https://github.com/facebook/react-native/blob/main/packages/rn-tester/Podfile#L27 ``` # Hermes is now enabled by default. # The following line will only disable Hermes if the USE_HERMES envvar is SET to a value other than 1 (e.g. USE_HERMES=0). hermes_enabled = !ENV.has_key?('USE_HERMES') || ENV['USE_HERMES'] == '1' ``` Build command: `USE_HERMES=1 bundle exec pod install` This will install the Hermes runtime Pod (not build it from scratch) & thus enable the `RCT_USE_HERMES` macro. https://www.internalfb.com/code/fbsource/[9f57823a75a40d3f8559c8f1b7ae0add8e95d6dc]/xplat/js/react-native-github/packages/rn-tester/RNTester/AppDelegate.mm?lines=10-16 --- The documentation for enabling Hermes on RN Desktop macOS are outdated: https://microsoft.github.io/react-native-windows/docs/hermes#hermes-on-macos > Install the npm package yarn add 'hermes-engine-darwin@^0.4.3' * `hermes-engine-darwin` is no longer required > Add (or uncomment) the following pod dependencies to your macOS target in your Podfile: pod 'React-Core/Hermes', :path => '../node_modules/react-native-macos/' pod 'hermes', :path => '../node_modules/hermes-engine-darwin' pod 'libevent', :podspec => '../node_modules/react-native-macos/third-party-podspecs/libevent.podspec' * Setting `USE_HERMES=1` during `pod install= replaces all of this > Copy Run pod install Be sure to set your target's deployment target to at least 10.14 before running pod install * `USE_HERMES=1 bundle exec pod install --verbose` --- On RN Desktop, the Hermes flag was [set to false](microsoft#780) due to M1 build reasons which have since been resolved. - microsoft#952 - microsoft#781 Curiously, the `RNTester-macOS` target AppDelegate was never updated to import & use Hermes when `RCT_USE_HERMES` was `true`. Only the `RNTester` for mobile had the correct Hermes usage. **RNTester-macOS:** https://github.com/microsoft/react-native-macos/blob/main/packages/rn-tester/RNTester-macOS/AppDelegate.mm **RNTester:** https://github.com/microsoft/react-native-macos/blob/main/packages/rn-tester/RNTester/AppDelegate.mm **Change** * Remove `pods(:hermes_enabled => true)` in favor of passing `USE_HERMES=1` to `pod install` (This is how it's done on RNTester iOS) * Copy Hermes support to `RNTester-macOS` AppDelegate Test Plan: **Install from scratch** Differential Revision: https://phabricator.intern.facebook.com/D38277077 Co-authored-by: Shawn Dempsey <shawndempsey@fb.com>

add optional callbacks to routing functions

4df6f10

reactjs-bot added the GH Review: review-needed label May 16, 2015

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 16, 2015

vjeux assigned ericvicenti May 16, 2015

pwmckenna added 2 commits May 20, 2015 14:23

Merge branch 'master' into callbacks

dacc937

add callback support to all other public methods

500aef1

ericvicenti reviewed May 26, 2015
View reviewed changes

pwmckenna added 2 commits May 26, 2015 10:54

fix _transitionTo arguments

a8b57b7

Merge branch 'master' into callbacks

9fa7776

brentvatne changed the title ~~add optional callbacks to routing functions~~ [Navigator] add optional callbacks to routing functions Jun 1, 2015

ericvicenti closed this Jun 9, 2015

ayushjainrksh pushed a commit to MLH-Fellowship/react-native that referenced this pull request Jul 2, 2020

Mix markup related to React Conf (facebook#1308)

09357ed

Add comma and space

ebarahona mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities ebarahona/react-native#68

Open

enterstudio mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities enterstudio/react-native#33

Open

admmasters mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities admmasters/react-native#54

Open

bumplzz69 mentioned this pull request May 13, 2022

[Snyk] Security upgrade yargs from 3.32.0 to 13.2.4 bumplzz69/react-native#119

Open

snyk-bot mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities massapina/react-native#33

Open

This was referenced May 14, 2022

[Snyk] Security upgrade yargs from 3.32.0 to 13.2.4 mdcollab/react-native#3

Open

[Snyk] Fix for 1 vulnerabilities mdcollab/react-native#6

Open

bumplzz69 mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities bumplzz69/react-native#122

Open

joseroubert08 mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities joseroubert08/react-native#51

Open

maximejf42 mentioned this pull request May 14, 2022

[Snyk] Security upgrade yargs from 3.32.0 to 13.2.4 maximejf42/react-native#30

Open

harunpehlivan mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities harunpehlivan/react-native#52

Open

caschomburg123 mentioned this pull request May 17, 2022

[Snyk] Fix for 2 vulnerabilities caschomburg123/react-native#50

Open

arealmaas mentioned this pull request May 17, 2022

[Snyk] Fix for 1 vulnerabilities EdtechFoundry/react-native#31

Open

snyk-community mentioned this pull request May 17, 2022

[Snyk] Fix for 2 vulnerabilities snyk-community/react-native#33

Open

snyk-bot mentioned this pull request May 19, 2022

[Snyk] Fix for 1 vulnerabilities AbhinavJain13/react-native#34

Open

snyk-bot mentioned this pull request Jun 22, 2022

[Snyk] Fix for 2 vulnerabilities harunpehlivan/react-native#54

Open

bumplzz69 mentioned this pull request Sep 27, 2022

[Snyk] Fix for 47 vulnerabilities bumplzz69/react-native#130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Navigator] add optional callbacks to routing functions #1308

[Navigator] add optional callbacks to routing functions #1308

pwmckenna commented May 16, 2015

ericvicenti commented May 19, 2015

pwmckenna commented May 20, 2015

ericvicenti commented May 20, 2015

pwmckenna commented May 20, 2015

ericvicenti commented May 20, 2015

ide commented May 20, 2015

pwmckenna commented May 20, 2015

ericvicenti commented May 21, 2015

ericvicenti May 26, 2015

pwmckenna May 26, 2015

ericvicenti commented Jun 8, 2015

ide commented Jun 8, 2015

[Navigator] add optional callbacks to routing functions #1308

[Navigator] add optional callbacks to routing functions #1308

Conversation

pwmckenna commented May 16, 2015

ericvicenti commented May 19, 2015

pwmckenna commented May 20, 2015

ericvicenti commented May 20, 2015

pwmckenna commented May 20, 2015

ericvicenti commented May 20, 2015

ide commented May 20, 2015

pwmckenna commented May 20, 2015

ericvicenti commented May 21, 2015

ericvicenti May 26, 2015

Choose a reason for hiding this comment

pwmckenna May 26, 2015

Choose a reason for hiding this comment

ericvicenti commented Jun 8, 2015

ide commented Jun 8, 2015