Allow changing mode in navigationOptions for dynamic modal #2308

JulianKingman · 2017-08-03T23:44:34Z

I need dynamic modals in my project and couldn't wait for #1378. It looked simple enough to implement, so I went ahead with it :)

I've got navigation animations working, but I'm having a heck of a time configuring the "back" animation.

Any idea where I can configure the navigator in such a way that I can influence the animation, and have per-screen navigationOptions available? As far as I can tell, isModal is currently defined in CardStackTransitioner here: https://github.com/react-community/react-navigation/blob/master/src/views/CardStack/CardStackTransitioner.js#L77, which seems to apply isModal to the navigator universally.

JulianKingman · 2017-08-07T14:22:41Z

Turns out I was wrong, setting isModal in CardStackTransitioner doesn't do the trick either, so I have no idea where the back animation comes from if not there, I can't find any reference to it.

davojan · 2017-08-08T14:02:13Z

@JulianKingman Back animation seems to be a separate issue. Even without your changes it's behaviour is odd in "Modal Stack Example" - "Go to profile screen" is animated vertically, but back-animation is horizontal.

In your modified example back navigation starts to work as expected if you click "Go to a photos screen" one more deeper and then click "Go back".

davojan · 2017-08-08T19:02:27Z

Nope, I was wrong. In "Modal Stack Example" there is a nested navigator that causing unintuitive behaviour.

But the good news is I made it to work correctly during back navigation! Check this out:
procraft@fa2a383#diff-95db94c88696779a7ac6a0fe1af87ebeR390

JulianKingman · 2017-08-23T17:18:30Z

Brilliant, thanks @davojan! I added the fix to my PR.

JulianKingman · 2017-08-23T21:27:08Z

Test thoroughly and describe how you tested it
I added examples to the NavigationPlayground and tested a modal type screen in a card stack, and a card type screen in a modal
Add an example to the playground app that utilizes this new option
Yes, done (see SimpleStack and ModalStack for examples)
Take screenshots/videos of the behavior on both platforms
Here:
Write documentation
Done

JulianKingman · 2017-08-23T21:28:25Z

@ericvicenti FYI, ready for review

davojan · 2017-08-28T16:24:38Z

@JulianKingman I guess you should fix linter errors reported by the circleci before they'll consider reviewing :)

JulianKingman · 2017-08-28T22:59:22Z

Fixed.

claykohut · 2017-08-31T17:01:00Z

Getting this error when trying to use this fork:

maxstoller · 2017-09-11T16:33:10Z

Working well for me, would be nice to see it merged. Thanks for sharing @JulianKingman!

kelset · 2017-09-11T16:47:05Z

Looks like there are some conflicts, can you fix them? I'll try to check the PR asap

examples/NavigationPlayground/package.json

@@ -15,7 +15,7 @@
    "flow-bin": "^0.49.1",
    "react": "16.0.0-alpha.12",
    "react-native": "^0.45.1",
-    "react-navigation": "*"
+    "react-navigation": "designmanio/react-navigation"


dccarmo · 2017-10-23T22:21:51Z

Any updates on this? It seems the conflicts are minimal (documentation and dependencies), so I guess it shouldn't be too hard to update and approve it?

helielson · 2018-02-01T03:03:29Z

@JulianKingman good job.
Are you having any issues to update this branch?

.expo/settings.json

+  "minify": false,
+  "urlType": "exp",
+  "urlRandomness": null
+}


codecov-io · 2018-02-01T14:37:30Z

Codecov Report

Merging #2308 into master will increase coverage by 0.07%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #2308      +/-   ##
==========================================
+ Coverage   72.75%   72.83%   +0.07%     
==========================================
  Files          53       53              
  Lines        1659     1671      +12     
  Branches      435      439       +4     
==========================================
+ Hits         1207     1217      +10     
- Misses        357      359       +2     
  Partials       95       95

Impacted Files	Coverage Δ
src/routers/createConfigGetter.js	`96.42% <ø> (ø)`	⬆️
src/TypeDefinition.js	`0% <ø> (ø)`	⬆️
src/views/CardStack/CardStack.js	`44.61% <100%> (+2.21%)`	⬆️
src/views/CardStack/CardStackTransitioner.js	`81.25% <75%> (-7.64%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7141f66...8e62e01. Read the comment docs.

examples/NavigationPlayground/js/ModalStack.js

@@ -14,32 +14,44 @@ const MyNavScreen = ({ navigation, banner }) => (
        top: navigation.state.routeName === 'HeaderTest' ? 'always' : 'never',
      }}
    >
-      <SampleText>{banner}</SampleText>
+    <SampleText>{banner}</SampleText>


examples/NavigationPlayground/js/SimpleStack.js

        <Button onPress={() => navigation.goBack(null)} title="Go back" />
        <StatusBar barStyle="default" />
      </SafeAreaView>
    );
  }
 }

+const Modal = ({navigation}) => (
+  <MyNavScreen
+    banner={`Modal in Screen Navigator`}


examples/NavigationPlayground/package.json

@@ -14,7 +14,7 @@
    "expo": "^25.0.0",
    "react": "16.2.0",
    "react-native": "^0.52.0",
-    "react-navigation": "link:../.."
+    "react-navigation": "file:../.."


helielson · 2018-02-01T15:02:10Z

@spencercarli @stereodenis @Andreyco @kelset the branch is now updated.
Can you review it again?

kelset · 2018-02-01T15:33:36Z

I think @ericvicenti should be the one reviewing this because we are undergoing some massive changes so I'm not sure this will be needed

raarts · 2018-02-01T16:33:33Z

we are undergoing some massive changes

is that documented somewhere, this makes me worried.

kelset · 2018-02-01T17:01:25Z

You can check the commits on master. We are not yet ready release a new version / post the release notes because we are still changing stuff, but yeah PR section & commits in master are the best way to be up-to-date if you don't want to wait for next release.

ericvicenti

One concern is that horizontal (non-modal) mode is super tricky, because you need to animate the previous screen at the same time as the active scene. So I think we do want to support this use case, but I'm not sure that hacking on CardStack is the best way to do it.

src/views/CardStack/CardStackTransitioner.js

+    if (navigation.state.routes && navigation.state.routes[1]) {
+      screenOptions = router.getScreenOptions(
+        addNavigationHelpers({
+          state: navigation.state.routes[1],


ericvicenti · 2018-02-01T18:06:00Z

And yes, as @kelset mentioned, we do have some big changes happening in this area. It should make features like this easier to add, moving forward!

See the suggested changes here: #3392
and the RFC here: https://github.com/react-navigation/rfcs/blob/master/text/0002-navigator-view-api.md

@raarts , we are ramping up support, it shouldn't concern you! Feel free to follow along with progress on the issues and PRs in this repo, and by keeping an eye on the RFCs repo: https://github.com/react-navigation/rfcs

This property very useful for customising TabBar look, but available only on Android. This commit add support for it for iOS.

Support iconStyle Tab Bar property on iOS

RobPando · 2018-02-09T13:40:52Z

My bad for the intrusion, but is this still in consideration to merge in or similar future solution? I need this bad.

brentvatne · 2018-02-09T17:41:04Z

I agree with @ericvicenti's assessment here, I'm not sure that this is the way we want to go for supporting modals. It may be, but I'd like to discuss further and have a proper RFC for this as it's a really key piece of the ergonomics of a navigation library (that we have up until now not provided a well-defined and supported path for, aside from creating a modal stack navigator). Please see this RFC placeholder issue: react-navigation/rfcs#15 -- if you want to take a stab at designing a solution to this, let's discuss there and we can take it into a formal RFC. Thank you @JulianKingman for this PR and creating good discussion around this, and sorry to everyone who has to continue to use a fork to support this feature!

Julian Kingman added 7 commits August 3, 2017 18:05

testing scene props

6f7ce9c

testing scene props

295bfb6

testing more props

d24f4d7

testing custom mode

9230451

get options properly

319c3c8

get by function

73a8af5

trying to implement back modal

b6199c2

JulianKingman mentioned this pull request Aug 7, 2017

getScreenForRouteName without routeConfig #2328

Closed

fix for back behavior

f5147f6

Julian Kingman added 2 commits August 23, 2017 17:16

fixed example

0e2fda8

added documentation

239ffd7

Julian Kingman added 3 commits August 28, 2017 17:10

prettier

9ce412b

Merge remote-tracking branch 'upstream/master'

748e6f8

fix flow errors

88f83de

davojan mentioned this pull request Aug 29, 2017

Maintain own fork of react-navigation? aksonov/react-native-router-flux#2276

Closed

charpeni reviewed Sep 16, 2017

View reviewed changes

examples/NavigationPlayground/package.json Outdated

@@ -15,7 +15,7 @@

"flow-bin": "^0.49.1",

"react": "16.0.0-alpha.12",

"react-native": "^0.45.1",

"react-navigation": "*"

"react-navigation": "designmanio/react-navigation"

This comment was marked as abuse.

Sign in to view

Julian Kingman added 2 commits October 27, 2017 07:11

fix conflicts

3e8dd0c

revert postinstall fix

9f28441

kelset mentioned this pull request Jan 2, 2018

React Navigation API Refresh #3195

Closed

kelset mentioned this pull request Jan 15, 2018

StackNavigator navigate to screen like modal #3273

Closed

Julian Kingman added 8 commits February 1, 2018 08:15

fix mode choice

175f468

Merge branch 'master' of https://github.com/DesignmanIO/react-navigation

8c96a4a

merge conflicts

22e7e76

clean up and fix code

cde1bb8

fix getScreenOptions

124b928

rm console

6b54e0f

cleanup

fc977ee

lint errors

ecd0eef

helielson reviewed Feb 1, 2018

View reviewed changes

.expo/settings.json Outdated

"minify": false,

"urlType": "exp",

"urlRandomness": null

}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

helielson reviewed Feb 1, 2018

View reviewed changes

Julian Kingman added 2 commits February 1, 2018 09:53

formatting, etc...

0704c42

update gitignore for expo

8e62e01

ericvicenti requested changes Feb 1, 2018

View reviewed changes

src/views/CardStack/CardStackTransitioner.js

if (navigation.state.routes && navigation.state.routes[1]) {

screenOptions = router.getScreenOptions(

addNavigationHelpers({

state: navigation.state.routes[1],

This comment was marked as abuse.

Sign in to view

SoundBlaster and others added 2 commits February 7, 2018 18:17

Support iconStyle Tab Bar property on iOS

e3a1e99

This property very useful for customising TabBar look, but available only on Android. This commit add support for it for iOS.

Merge pull request #1 from SoundBlaster/patch-2

a3a2c10

Support iconStyle Tab Bar property on iOS

brentvatne mentioned this pull request Feb 9, 2018

Improve ergonomics of overlays / popovers / modals react-navigation/rfcs#15

Open

brentvatne closed this Feb 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow changing mode in navigationOptions for dynamic modal #2308

Allow changing mode in navigationOptions for dynamic modal #2308

JulianKingman commented Aug 3, 2017 •

edited

JulianKingman commented Aug 7, 2017

davojan commented Aug 8, 2017

davojan commented Aug 8, 2017

JulianKingman commented Aug 23, 2017

JulianKingman commented Aug 23, 2017

JulianKingman commented Aug 23, 2017

davojan commented Aug 28, 2017

JulianKingman commented Aug 28, 2017

claykohut commented Aug 31, 2017 •

edited

maxstoller commented Sep 11, 2017 •

edited

kelset commented Sep 11, 2017

This comment was marked as abuse.

dccarmo commented Oct 23, 2017

helielson commented Feb 1, 2018

This comment was marked as abuse.

This comment was marked as abuse.

codecov-io commented Feb 1, 2018 •

edited

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

helielson commented Feb 1, 2018

kelset commented Feb 1, 2018

raarts commented Feb 1, 2018

kelset commented Feb 1, 2018

ericvicenti left a comment

This comment was marked as abuse.

ericvicenti commented Feb 1, 2018

RobPando commented Feb 9, 2018

brentvatne commented Feb 9, 2018

Allow changing mode in navigationOptions for dynamic modal #2308

Allow changing mode in navigationOptions for dynamic modal #2308

Conversation

JulianKingman commented Aug 3, 2017 • edited

JulianKingman commented Aug 7, 2017

davojan commented Aug 8, 2017

davojan commented Aug 8, 2017

JulianKingman commented Aug 23, 2017

JulianKingman commented Aug 23, 2017

JulianKingman commented Aug 23, 2017

davojan commented Aug 28, 2017

JulianKingman commented Aug 28, 2017

claykohut commented Aug 31, 2017 • edited

maxstoller commented Sep 11, 2017 • edited

kelset commented Sep 11, 2017

This comment was marked as abuse.

dccarmo commented Oct 23, 2017

helielson commented Feb 1, 2018

This comment was marked as abuse.

This comment was marked as abuse.

codecov-io commented Feb 1, 2018 • edited

Codecov Report

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

helielson commented Feb 1, 2018

kelset commented Feb 1, 2018

raarts commented Feb 1, 2018

kelset commented Feb 1, 2018

ericvicenti left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

ericvicenti commented Feb 1, 2018

RobPando commented Feb 9, 2018

brentvatne commented Feb 9, 2018

JulianKingman commented Aug 3, 2017 •

edited

claykohut commented Aug 31, 2017 •

edited

maxstoller commented Sep 11, 2017 •

edited

codecov-io commented Feb 1, 2018 •

edited