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

inherit props #135

Merged
merged 16 commits into from Apr 26, 2018

Conversation

Projects
None yet
6 participants
@jljsj33
Member

jljsj33 commented Apr 20, 2018

No description provided.

@jljsj33 jljsj33 requested a review from afc163 Apr 20, 2018

@jljsj33 jljsj33 changed the title from inherit props to [WIP]inherit props Apr 20, 2018

@jljsj33 jljsj33 changed the title from [WIP]inherit props to inherit props Apr 20, 2018

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2018

Coverage Status

Coverage increased (+0.01%) to 99.675% when pulling 0e2b180 on inherit-props into e688ac2 on master.

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 20, 2018

好了 @afc163

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 21, 2018

@jljsj33 能提供一些背景么?没太懂

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 21, 2018

data-*

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 21, 2018

我这边 landings 编辑时是用 props 传递,用到了 data-id 和其他 data-之类的。。朱兴民叫我在外层套 div ,那只能获取 Menu 外层参数,Item 就不能获取。。。更何况(只我的想法)组件本来就应该继承 props,更方便让别人使用,以前这个组件连 style 都不带,那别人想稍改样式都不可以。最好还是带上吧。。

@@ -206,19 +206,22 @@ const MenuMixin = {
domProps.tabIndex = '0';
domProps.onKeyDown = this.onKeyDown;
}
const { prefixCls, eventKey, visible } = props;
menuInheritProps.forEach(key => delete props[key]);

This comment has been minimized.

@picodoth

picodoth Apr 21, 2018

Contributor

如果这个地方的意思是说,menu自己的props不往下传,我觉得应该把menuInheritProps改名成menuProps就好了

This comment has been minimized.

@jljsj33

jljsj33 Apr 21, 2018

Member

可能这名称不好。。。我是把所有导航里的props提到一个地方。。因为很多重复的。。每个都跑一遍也没关系。。。

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 21, 2018

@jljsj33 你这样就是给底层元素提供一种传入props的后门是吧,我觉得挺hack的。比如我想在menu上加一个比如跟padding有关的style,结果就直接传到<li>了? @afc163 看看?

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 21, 2018

额。。。li上的style应该是在Item上吧。。。加这个的意思是把dom的所有可添加的参数都可传到组件上

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 22, 2018

你现在的这个做法,menu上的style就是会传到li上, 因为你没有把style从props里过滤掉啊。我觉得如果你真的要传自己额外的数据,集中放在一个props上不行么,比如叫configData啥的,就不用这么搞了

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 22, 2018

???? 别蒙我。。怎么可能把 menu 的 style 传到 li上?

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 22, 2018

别逗我。。。@afc163 合掉。发个版本。。

@picodoth

This comment has been minimized.

Contributor

picodoth commented Apr 22, 2018

我的错,以为所有menu上的props都自动传下去了。别的没啥问题了,menuInheritProps那个地方还是觉得怪怪的。考虑一下直接通过Menu组件获取props (Menu.propTypes)? 否则后面props有修改还得回过头来改menuInheritProps?

@@ -165,8 +165,10 @@ export const MenuItem = createReactClass({
if (props.mode === 'inline') {
style.paddingLeft = props.inlineIndent * props.level;
}
menuAllProps.forEach(key => delete props[key]);

This comment has been minimized.

@afc163

afc163 Apr 23, 2018

Member

这样 MenuItem 上的 onClick、onMouseEnter、onMouseLeave 就没了,这几个可以有的。

MenuItemGroup 和 SubMenu 同理。

This comment has been minimized.

@jljsj33

jljsj33 Apr 23, 2018

Member

不对啊,,,这些事件不是在内部重写了吗???那还带上干啥???
有 { ...mouseEvent } 啊...

This comment has been minimized.

@jljsj33

jljsj33 Apr 23, 2018

Member

好像只有 group 上有用,,加着吧。。。

@afc163

This comment has been minimized.

Member

afc163 commented Apr 23, 2018

冲突了,可能要重新改过。

@afc163

This comment has been minimized.

Member

afc163 commented Apr 23, 2018

几个子组件的 onClick 顺便支持一下吧。

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 24, 2018

我日,,什么鬼?

@jljsj33 jljsj33 force-pushed the inherit-props branch from ae1ffa7 to 95a42e1 Apr 24, 2018

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 24, 2018

props 带下去了,,子组件里默认有这几个鼠标事件吧。。。

@yesmeck

This comment has been minimized.

Member

yesmeck commented Apr 24, 2018

把这里

menu/src/SubPopupMenu.js

Lines 259 to 284 in e688ac2

const newChildProps = {
mode: props.mode,
level: props.level,
inlineIndent: props.inlineIndent,
renderMenuItem: this.renderMenuItem,
rootPrefixCls: props.prefixCls,
index: i,
parentMenu: props.parentMenu,
// customized ref function, need to be invoked manually in child's componentDidMount
manualRef: childProps.disabled ? undefined :
createChainedFunction(child.ref, saveRef.bind(this, i)),
eventKey: key,
active: !childProps.disabled && isActive,
multiple: props.multiple,
onClick: this.onClick,
onItemHover: this.onItemHover,
openTransitionName: this.getOpenTransitionName(),
openAnimation: props.openAnimation,
subMenuOpenDelay: props.subMenuOpenDelay,
subMenuCloseDelay: props.subMenuCloseDelay,
forceSubMenuRender: props.forceSubMenuRender,
onOpenChange: this.onOpenChange,
onDeselect: this.onDeselect,
onSelect: this.onSelect,
...extraProps,
};
从 Menu 传给 MenuItem 的 props group 到 menuProps 好点吧

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 24, 2018

@yesmeck 啥意思,,没懂

@yesmeck

This comment has been minimized.

Member

yesmeck commented Apr 24, 2018

就是改成这样:

const menuProps = {
  mode: props.mode,
  level: props.level,
  ...
};
return React.cloneElement(child, { menuProps });

这样就不需要再维护一个 props 列表去 MenuItem 里删除 Menu 的 props。

@jljsj33

This comment has been minimized.

Member

jljsj33 commented Apr 25, 2018

@afc163 可以合了啊。。。

@afc163 afc163 merged commit c9c17ed into master Apr 26, 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 (+0.01%) to 99.675%
Details

@afc163 afc163 deleted the inherit-props branch Apr 26, 2018

@afc163

This comment has been minimized.

Member

afc163 commented Apr 26, 2018

picodoth added a commit that referenced this pull request Apr 28, 2018

@blog-lyn

This comment has been minimized.

Contributor

blog-lyn commented May 23, 2018

好坑啊 我们是把props都传到item里面使用,但是不想Create到 dom节点上啊,现在就会报错:

React does not recognize the `TreeNode` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `treenode` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
    in li (created by SubMenu)
    in SubMenu (created by Connect(SubMenu))
    in Connect(SubMenu) (created by SubMenu)
    in SubMenu (created by TreeNode)
    in TreeNode (created by DragSource(TreeNode))

难道再增加一个配置项, 把不想增加到 item上的属性写在一个list里面,在list里面的prop delete掉?

@picodoth

This comment has been minimized.

Contributor

picodoth commented May 23, 2018

@blog-lyn 什么情况下会把TreeNode这样的特殊属性传给MenuItem用呢,或者反过来说MenuItem除了spread到li之外,如何使用这样的特殊props? 如果你说的用例确实是有效用例,那我们可能得考虑在MenuItem单独开一个新的props来存放希望传到li上的属性。

@blog-lyn

This comment has been minimized.

Contributor

blog-lyn commented Jul 17, 2018

@picodoth
我们的menu是一个树结构, 大小最多会有几百个节点,
为了在每个menu.item render 显示不同的名字 和绑定不同的事件,我们需要除了menu.item key 以外的属性.

因此我们在处理事件的时候需要直接拿到对应的menu.item的data.

  <Menu.Item  {...this.props}>
        xxxx
  </Menu.Item>

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