[NavigationExperimental] Render overlay first in NavigationAnimatedView

[tlvenn]

At the moment, the overlay is the last element rendered in the AnimationAnimatedView:
https://github.com/facebook/react-native/blob/master/Libraries/NavigationExperimental/NavigationAnimatedView.js#L165

The drawback of this approach is you have to position the overlay in absolute to get it on top and then you have to manually add a marginTop to your scene if the overlay is not a true overlay, like the navigation header.

Given RN flexbox implementation does not support order property, we cannot re arrange it to be first. I dont think there is any drawback to actually renderer it first so that we can naturally leverage flexbox if needed.

@ericvicenti any though ?

Thanks

[ericvicenti]

We still need to be able to position the header above everything else. I think it may be appropriate to also have an underlay which works in the same way as the overlay but is rendered before any scenes.

@hedgerwang, any thoughts?

[tlvenn]

I assumed that you would actually use the overlay to inject the header and it would therefore naturally be above everything else. Am missing something ?

[ericvicenti]

We still need to be able to overlay the header above the scene because in some apps (usually iOS), the header is partially transparent or blurry, and it is expected to see the scene scrolling behind it.

[tlvenn]

But if you position the overlay in absolute, don't you already get that, independently of the overlay being rendered first or not ? The render flow/order should not matter anymore.

[tlvenn]

Ha i guess you are talking about the z-index and by rendered last you make sure it's above (elevation) the scene...

[tlvenn]

So In the scenario that the overlay is solid, being able to underlay rather than overlay would be pretty handy.

[ericvicenti]

Yeah, that makes sense. So I would be in support of a new renderUnderlay prop. But I understand that naming is confusing. If this is important to you, can you file a new issue about the naming and propose some alternate names?

[tlvenn]

Sure thing, thanks for the quick feedback !

[tlvenn]

I also wonder that given you dont really want to have both an underlay and an overlay, maybe the solution would be to introduce a boolean prop renderOverlayFirst to indicate if the overlay should be rendered first or not. Or the reverse, renderOverlayLast with default value to true.

What do you think ?

[tlvenn]

But that does leave things a little bit weird with an overlay that potentially ends up not being technically over...

[ericvicenti]

I think there may be some extreme use cases where developers may want both an overlay and an underlay, and it is easy to support both, so I think we should probably do that.

Another thing to consider: we could remove the overlay entirely from this view and let the app developer do this part themselves, or use a higher-level component that has the desired behavior

[hedgerwang]

@tlvenn : could you please try to move the overlay before the scenes and remove the absolute position styles and see whether that would work with multiple scenes? I think it makes senses to not enforce fixed-height header and margin-top.

@ericvicenti: I think overlay could be removed off the AnimatedView. Otherwise we'd have to add more overlays for footer, drawer, sidebar....etc.

We could decouple the API to be something like this:

<View>
  <NavigationAnimatedView navigationState={navigationState} renderScene={renderHeader} />
  <NavigationAnimatedView navigationState={navigationState} renderScene={renderScene} />
</View>

[ericvicenti]

@hedgerwang, that is problematic because on iOS the header needs to animate at the same speed as the gesture that is happening on the scene

[tlvenn]

@hedgerwang It does work just fine as far as your overlay is in the flow and push downward the scene but as pointed out by @ericvicenti , if you want your overlay to float above your scene, out of its flow, if you render it first, it will be behind the scene (z-index) instead of being on top of it.

[hedgerwang]

@ericvicenti : you're right! I forgot about the fact that the position is owned by the NavigationAnimatedView` and sharing them with two animated view could be tricky.

[hedgerwang]

@tlvenn : I shall play around the idea by remove the position: absolute styles. If it works, I'll make the change.

[tlvenn]

@hedgerwang : But given z-index in RN is provided by render order, if you render the overlay first, it will necessary be under the scene and then the scenario that @ericvicenti has mentioned cannot be done anymore.

And yes , thinking about it, it could be useful to have both underlay and overlay. For example, I would use the underlay to render the navigation header using flexbox and I could leverage the overlay to display pure JS modals.

[tlvenn]

I think probably the best course of action would be to remove the position: absolute styles from the NavigationHeader container with the expectation that it would leverage the underlay rather than the overlay. And if people needs something fancy with transparent / semi transparent nav where the scene can scroll under, they can still do it by overriding the nav header styles to use absolute positioning and then leveraging the overlay.

Benefit would be that for most cases, the navigation header will automatically push downward the scene without asking the user to duplicate the nav header height calculation logic on his own so he can use it as a marginTop to his scenes.

[tlvenn]

hi @hedgerwang, any update ? Thanks

[jmurzy]

@hedgerwang @ericvicenti What's the final verdict on this one? Seems like we soon will have z-index support. Given that, I'm not sure what value there is in adding a renderUnderlay prop. But if we still want to go ahead with this, I can send in a PR that adds it to both (now deprecated) AnimatedView and Transitioner.

🍺

[jmurzy]

This issue was addressed in 3a62314. Both renderScene and renderOverlay are now deprecated. NavigationTransitioner now delegates rendering of all its children to a user provided render function in which you can place your overlay however you want.

🍺

[ide]

Closing this based on @jmurzy's comment. Thanks for helping out with all the Navigation diffs & issues btw :)

[ericvicenti]

Yeah I think we can close this issue now, as we are about to deprecate NavigationAnimatedView.

NavigationTransitioner isn't just a better name for NavigationAnimatedView, its a simpler implementation that avoids issues like this entirely. As @jmurzy said, you are now responsible for rendering all of the scenes as well as the header/footer/underlay. Instead of renderScene, renderOverlay and renderUnderlay, you just have render.

Instead of this:

<NavigationAnimatedView
  renderScene={this._renderScene}
  renderOverlay={this._renderOverlay}
  navigationState={ns}
/>

We have this:

<NavigationTransitioner
  render={(props) => (
    <View>
      {props.scenes.map(scene => this._renderScene({ scene }))}
      {this._renderOverlay(props)}
    </View>
  )}
  navigationState={ns}
/>

So hopefully this will enable people to make more creative views that allow crazy integrations between the header and scenes. We can also build better tools and abstractions by having a more flexible interface and allowing the transitioner to do what it does best.

[farzd]

I'm not sure i understand this issue correctly but is it to do with the z-index of the navigation bar? i'm using vanilla navigator and am having issues with content obviously being on a lower z-index order [i.e swipeable tinder cards are behind navigation bar] can i get around this by using Navigation experimental?