Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NavigationExperimental: Unnecessary rendering in NavigationView. #6579

Closed
5amfung opened this issue Mar 22, 2016 · 3 comments
Closed

NavigationExperimental: Unnecessary rendering in NavigationView. #6579

5amfung opened this issue Mar 22, 2016 · 3 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@5amfung
Copy link

5amfung commented Mar 22, 2016

While I was testing out and tried to understand how NavigationCompositionExample works, I found that renderScene() was called 3 times (exactly the number of tabs) for a single view. If you have N tabs, you will see N x N times of the debug log. I don't quite understand it. It doesn't seem it's necessary.

class ExampleMainView extends React.Component {
  render() {
    return (
      <NavigationView
        navigationState={this.props.navigationState}
        style={styles.tabsContent}
        renderScene={(tabState, index) => {
          console.log('index:', index);  // <<<<< This is printed 3x.
          return (
            <ExampleTabScreen
              key={tabState.key}
              navigationState={tabState}
              onNavigate={this._handleNavigation.bind(this, tabState.key)}
            />
          );
        }}
      />
    );
  }
  //...
}

The output:

'index:', 0
'index:', 1
'index:', 2

I think, the culprit is render() in NavigationView. It runs map() over children. But why?

var NavigationView = React.createClass({
  propTypes: {
    // todo, figure out a propType for getK
    navigationState: React.PropTypes.object.isRequired,
    renderScene: React.PropTypes.func.isRequired,
  },
  render: function() {
    return (
      <View
        style={this.props.style}>
        {this.props.navigationState.children.map(this._renderScene)}
      </View>
    );
  },
  _renderScene: function(route, index) {
    var isSelected = index === this.props.navigationState.index;
    return (
      <View
        key={route.key}
        pointerEvents={isSelected ? 'auto' : 'none'}
        style={[
          styles.navView,
          {opacity: isSelected ? 1 : 0},
        ]}>
        {this.props.renderScene(route, index)}
      </View>
    );
  }
}
@emrosenf
Copy link

The docs say NavigationView is "A simple view that will render a scene for the currently presented sub-state. The most common use-case is for tabs, where no transition is needed." Maybe @ericvicenti or @hedgerwang knows if we can remove map() and just render the current navigationState.index?

@hedgerwang
Copy link

Good catch.

The is no transition and we should just render the focused scene.
This file seems outdated to me and we should have updated it to ensure its design matches the rest of the component.

import type  {
  NavigationParentState,
  NavigationScene,
  NavigationSceneRenderer,
  NavigationSceneRendererProps,
} from 'NavigationTypeDefinition';

type Props = {
  navigationState: NavigationParentState,
  renderScene: NavigationSceneRenderer,
};

const {PropTypes} = React;

class NavigationView extends React.Component<any, Props, any> {
  static propTypes = {
    navigationState: NavigationPropTypes.navigationParentState,
    renderScene: PropTypes.func.isRequired,
  };

  render(): ReactElement {
    return (
      <View
        style={this.props.style}>
        {this._renderScene}
      </View>
    );
  }

  _renderScene(): ReactElement {
    const {navigationState} = this.props;

    const scenes = navigationState.children.map((childState, index) => {
      // @type NavigationScene.
      return {
        index,
        isStale: index !== navigationState.index,
        key: 'scene_' + childState.key,
        navigationState: childState,
      };
    });

    // @type NavigationSceneRendererProps.
    const props = {
      navigationState,
      layout: {
        height: new Animated.Value(0),
        initHeight: 0,
        initWidth: 0,
        width: new Animated.Value(0),
      },
      navigationState,
      onNavigate: this.props.onNavigate,
      position: new Animated.Value(navigationState.index),
      scene: scenes[navigationState.index],
      scenes,
    };

    return (
      <View
        style={[
          styles.navView,
          {opacity: isSelected ? 1 : 0},
        ]}>
        {this.props.renderScene(props)}
      </View>
    );
  }
}

ghost pushed a commit that referenced this issue Mar 29, 2016
Summary:This address the issue reported at
#6579 (comment)

NavigationView should have the same API as NavigationAnimatedView does except
that NavigationView does not need the APIs for animation.

This unify the API of our core components so that people can freely
compose views with both NavigationAnimatedView or NavigationView.

Reviewed By: fkgozali

Differential Revision: D3096076

fb-gh-sync-id: 7536777a7d637da62a2636d750f9d91c5a0eb45f
fbshipit-source-id: 7536777a7d637da62a2636d750f9d91c5a0eb45f
zebulgar pushed a commit to nightingale/react-native that referenced this issue Jun 18, 2016
Summary:This address the issue reported at
facebook#6579 (comment)

NavigationView should have the same API as NavigationAnimatedView does except
that NavigationView does not need the APIs for animation.

This unify the API of our core components so that people can freely
compose views with both NavigationAnimatedView or NavigationView.

Reviewed By: fkgozali

Differential Revision: D3096076

fb-gh-sync-id: 7536777a7d637da62a2636d750f9d91c5a0eb45f
fbshipit-source-id: 7536777a7d637da62a2636d750f9d91c5a0eb45f
@mkonicek
Copy link
Contributor

Hi there! This issue is being closed because it has been inactive for a while.

But don't worry, it will live on with ProductPains! Check out its new home: https://productpains.com/post/react-native/navigationexperimental-unnecessary-rendering-in-navigationview

ProductPains helps the community prioritize the most important issues thanks to its voting feature.
It is easy to use - just login with GitHub. GitHub issues have voting too, nevertheless
Product Pains has been very useful in highlighting the top bugs and feature requests:
https://productpains.com/product/react-native?tab=top

Also, if this issue is a bug, please consider sending a pull request with a fix.
We're a small team and rely on the community for bug fixes of issues that don't affect fb apps.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 20, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

5 participants