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

React Tabbar, update page when not passing explicit prop in. #2508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cjweigle
Copy link

@cjweigle cjweigle commented Aug 6, 2018

Issue #2335 is happening due to the Tabbar components internal page cache being incorrectly updated when the Page component returned from the renderTabs function changes.

In Tabbar's render method:

 const tabs = this.props.renderTabs(this.props.index, this);

 if (!this.tabPages) {
      this.tabPages = tabs.map((tab) => tab.content);
    } else {
      this.tabPages[this.props.index] = tabs[this.props.index].content;
    }

The first render of the component will populate tabPages with every tabs content. Subsequent renders will only update the current tab's content. A page's content fails to update when the 'index' prop is out of sync with the actual index that needs to be update. This can occur for two reasons:

  1. No 'index' prop is being passed in.
  2. The 'index' prop is being passed in and the user has swiped or clicked on a tab to navigate a different page.

So what happens in case 1? The default for the index prop is 0. As such, changes to the first tab's content will be made, all other tabs will be ignored.

Tabbar.defaultProps = {
  index: 0
};

In case 2, the 'index' prop can be out of sync with the ons-tabbar's active tab index since it can change by means other than just updating the Tabbar's 'index' prop.

In Tabbar's componentWillReceiveProps method:

 if (nextProps.index !== this.props.index && nextProps.index !== node.getActiveTabIndex()) {
      node.setActiveTab(nextProps.index, { reject: false });
    }

How do we solve this problem? The easiest solution would be to regenerate the entire tabPages cache on every render. This has the downside of removing the entire optimization and results in unnecessary reconciliation within react. A better solution is keeping the current pattern, but ensuring that the correct tab is updated.

Conceptually, what we want is this:

 const node = this._tabbar;
 const tabs = this.props.renderTabs(node.getActiveTabIndex(), this);

 if (!this.tabPages) {
      this.tabPages = tabs.map((tab) => tab.content);
    } else {
      this.tabPages[node.getActiveTabIndex()] = tabs[node.getActiveTabIndex()].content;
    }

Notice how the responsibility of deciding which tab's content gets updated has changed from the passed in 'index' prop to the ons-tabbar's current active tab.

There are 2 issues with this solution though:

  1. Changing ons-tabbar's active tab by a means internal to the web component (swiping / clicking a tab) will not cause react to rerender, and the tab's content will not be updated.
  2. It is not guaranteed that node.getActiveTabIndex() will return the value set in Tabbar's componentWillReceiveProps method by the time Tabbar's render method is called.

The first point seems like a non-issue, since renderTabs' output can't change without a rerender happening. It is an issue though. We can't be sure that renderTab's output hadn't changed during a prior rendering, which was thrown out by not updating tabPages to prevent unnecessary reconciliation.

The second point results from the possibility of setting ons-tabbar's active tab being asynchronous. This appears to be rare, but happens when:
A. A tab's page is not loaded.
B. platform.isUIWebView() is true.

To ensure that react rerenders the component and uses the correct tab index in response to the active tab changing, this patch tracks the active tab within Tabbar's state, which gets updated during the onPostChange event or in response to the 'index' prop changing. I have tested it with the example in issue #2335 to confirm it works, and I am unaware of any side-effects or bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant