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: refactor/remove mixin #133

Merged
merged 8 commits into from Apr 23, 2018

Conversation

Projects
None yet
5 participants
@picodoth
Contributor

picodoth commented Apr 18, 2018

  • Handle menu.onKeyDown usage in rc-select

@picodoth picodoth requested a review from yesmeck Apr 18, 2018

@coveralls

This comment has been minimized.

coveralls commented Apr 18, 2018

Coverage Status

Coverage increased (+1.8%) to 99.666% when pulling d8e200b on rm-mixin into 6eb19f0 on master.

@yesmeck

This comment has been minimized.

Member

yesmeck commented Apr 18, 2018

Remove all createReactClass, use ES class.

@picodoth picodoth force-pushed the rm-mixin branch from f16aec7 to 8529ac3 Apr 19, 2018

return true;
});
if (!this.props.defaultActiveFirst && activeIndex !== -1) {
if (allDisabled(children.slice(activeIndex, len - 1))) {

This comment has been minimized.

@afc163

afc163 Apr 19, 2018

Member

这两个 if 可以合并。

}
const start = (activeIndex + 1) % len;
let i = start;
for (; ;) {

This comment has been minimized.

@afc163

afc163 Apr 19, 2018

Member

用 while 是不是清晰一点。

if (i === start) {
return null;
}
} else {

This comment has been minimized.

@afc163

afc163 Apr 19, 2018

Member

这个 else 是多余的。

This comment has been minimized.

@picodoth

picodoth Apr 19, 2018

Contributor

不是多余的,如果找到一个有效的child就立马返回啊

if (c) {
this.instanceArray[index] = c;
}
}
const SubPopupMenu = createReactClass({

This comment has been minimized.

@afc163

afc163 Apr 19, 2018

Member

去掉了 Mixin 就可以用 ES6 class 了。

getStore() {
const store = this.props.store;
return store;

This comment has been minimized.

@afc163

afc163 Apr 19, 2018

Member

return this.props.store?

@picodoth picodoth changed the title from [WIP]chore: refactor/remove mixin to chore: refactor/remove mixin Apr 19, 2018

@picodoth picodoth force-pushed the rm-mixin branch from c6062c4 to 7cc542f Apr 19, 2018

@picodoth picodoth force-pushed the rm-mixin branch from 1ccfc29 to e1d27b9 Apr 19, 2018

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 19, 2018

@yesmeck @afc163 take another look

},
});
MenuItem.isMenuItem = 1;

This comment has been minimized.

@yesmeck

yesmeck Apr 19, 2018

Member

这句应该不用改

This comment has been minimized.

@picodoth

picodoth Apr 20, 2018

Contributor

不知道之前是不是埋了一个雷,isMenuItem应该放在包裹它的HOC上的,因为这里:
https://github.com/react-component/menu/blob/master/src/util.js#L40
我们都是直接用connectedComponent而不是menuItem

This comment has been minimized.

},
constructor(props) {
super(props);
}

This comment has been minimized.

@yesmeck

yesmeck Apr 19, 2018

Member

constructor 可以去掉

@@ -188,6 +188,7 @@ exports[`Menu render renders inline menu correctly 1`] = `
class="rc-menu-submenu-arrow"
/>
</div>
<div />

This comment has been minimized.

@yesmeck

yesmeck Apr 19, 2018

Member

为啥会多个 div 出来

This comment has been minimized.

@picodoth

picodoth Apr 20, 2018

Contributor

之前subpopubmenu会做一个渲染判断:
https://github.com/react-component/menu/blob/master/src/SubPopupMenu.js#L91
现在这部分逻辑(renderChildren)被提到subMenu了(因为Animation被提取到subMenu了)
https://github.com/react-component/menu/pull/133/files#diff-7f8a02620917aa2ed1113e338915793bR374
children的返回结果是要做Trigger的popup:
https://github.com/react-component/menu/pull/133/files#diff-7f8a02620917aa2ed1113e338915793bL449
popup属性不能为null,所以就返回了一个空div,这个不影响用户看到的渲染结果的

level: PropTypes.number,
mode: PropTypes.oneOf(['horizontal', 'vertical', 'vertical-left', 'vertical-right', 'inline']),
triggerSubMenuAction: PropTypes.oneOf(['click', 'hover']),
inlineIndent: PropTypes.number,

This comment has been minimized.

@yesmeck

yesmeck Apr 23, 2018

Member

这个原来有当成 string 用的。

This comment has been minimized.

@picodoth

picodoth Apr 23, 2018

Contributor

后面在用的时候是这么用的:
style.paddingLeft = props.inlineIndent * props.level;, 不过它会自动类型转化,为了跟已有的code类型兼容,我改成了:
inlineIndent: PropTypes.oneOfType(PropTypes.number, PropTypes.string),

@yesmeck

This comment has been minimized.

Member

yesmeck commented Apr 23, 2018

其他 ok 👍

@picodoth picodoth force-pushed the rm-mixin branch from d3396e9 to d8e200b Apr 23, 2018

@picodoth picodoth merged commit e688ac2 into master Apr 23, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.8%) to 99.666%
Details
@yesmeck

This comment has been minimized.

Member

yesmeck commented Apr 23, 2018

分支要删掉。

@afc163 afc163 deleted the rm-mixin branch Apr 23, 2018

@jljsj33

This comment has been minimized.

Member

jljsj33 commented on src/SubPopupMenu.js in d8e200b Apr 24, 2018

??? 不审核的???

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 24, 2018

@afc163

This comment has been minimized.

Member

afc163 commented Apr 27, 2018

@picodoth 可以发个大版本,然后更新到 antd feature-3.5.0 分支上,提前合上去发现问题。月底 3.5 上更新掉。

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 27, 2018

@afc163 好的

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment