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

chore: rm mixin and react-create-class #123

Merged
merged 8 commits into from
Jul 10, 2018
Merged

chore: rm mixin and react-create-class #123

merged 8 commits into from
Jul 10, 2018

Conversation

picodoth
Copy link
Contributor

@picodoth picodoth commented May 27, 2018

  • remove mixin
  • remove react-create-class
  • api stays the same and no test regression

Todo:
rm componentWillReceiveProps and componentWillUnMount.
Better do in a seperate PR;

@picodoth picodoth requested review from yesmeck and chenshuai2144 May 27, 2018 12:17
@coveralls
Copy link

coveralls commented May 27, 2018

Coverage Status

Coverage increased (+9.6%) to 73.198% when pulling 173e624 on rm-mixin2 into a6b82cd on master.

src/InkTabBar.js Outdated
this[name] = node;
}
};
}
Copy link
Member

@yesmeck yesmeck Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这些也可以抽出来:

export default function SwipeableInkTabBar() {
  return (
    <SaveRef>
      {({ save, get }) => (
        <TabBarRootNode saveRef={save} {...this.props}>
          <SwipeableTabBarNode saveRef={save} getRef={get} {...this.props}>
            <InkTabBarNode saveRef={save} getRef={get} {...this.props} />
            <TabBarSwipeableTabs saveRef={save} {...this.props} />
          </SwipeableTabBarNode>
        </TabBarRootNode>
      )}
    <SaveRef>
  );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@yesmeck
Copy link
Member

yesmeck commented Jun 13, 2018

kapture 2018-06-13 at 12 08 47

错乱了

@yesmeck
Copy link
Member

yesmeck commented Jun 13, 2018

可能要先把覆盖率提升下。

@picodoth
Copy link
Contributor Author

picodoth commented Jun 19, 2018

@yesmeck

错乱了

找到问题了,原来用的是 mixin , 生命周期都在同一个组件上,现在的模式把生命周期拆分了。InkTabBarNode 的 componenentDidUpdate 需要在 TabBarTabsNode 后面发生。

加了一个测试用例。再看下

@yesmeck
Copy link
Member

yesmeck commented Jul 3, 2018

antd 测试会失败。

screen shot 2018-07-03 at 16 09 32

screen shot 2018-07-03 at 16 10 38

@picodoth
Copy link
Contributor Author

picodoth commented Jul 3, 2018

前面那个有点奇怪,后面这个我晚上看一下

@picodoth
Copy link
Contributor Author

picodoth commented Jul 4, 2018

@yesmeck 看过了 diff 是无害的,
有两种:

一种是 inkbar 和 tabbar 顺序反了,这是因为 inkbar 依赖了 tabbar 的状态,所以需要把 inkbar 放在 tabbar 后面渲染,也就是这里的 https://github.com/react-component/tabs/pull/123/files#diff-ad3456d46a980c23853783634a1555a7R127

screen shot 2018-07-03 at 16 10 38

另外上面这个图的 diff 就是多了一堆中间状态节点,也是预期之中的

@yesmeck
Copy link
Member

yesmeck commented Jul 10, 2018

需要 rebase 下,把这个问题 #121 在这个分支上也修下。

@picodoth
Copy link
Contributor Author

rebase 完毕,看了下新的 diff 没啥问题,我合了!

@picodoth picodoth merged commit 0320545 into master Jul 10, 2018
@picodoth picodoth deleted the rm-mixin2 branch July 10, 2018 09:56
afc163 added a commit to ant-design/ant-design that referenced this pull request Jul 29, 2018
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.

3 participants