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

[WIP]fix: passing onclick through #141

Closed
wants to merge 1 commit into from
Closed

[WIP]fix: passing onclick through #141

wants to merge 1 commit into from

Conversation

picodoth
Copy link
Contributor

@picodoth picodoth commented May 5, 2018

#140 出现的问题inherit props导致的;而#135 想解决的问题是ant-design/ant-design#9004

因为我们用的是cloneElement的方法,所以inherit props 不得不把onClick事件一层层传下去,但是也正因为在中间层subMenu/subPopupMenu传递了onClick,所以才会触发不必要的冒泡,所以这里给出的解决方案是:
既然我们只是想把onClick传下去,那么在中间层重命名一下,不要叫onClick,这样就不会响应错误的冒泡事件了。

其实在MenuItem这一层做{...props}不已经解决了Menu.Item should have the onClick param?只要删去中间层(subMenu/subPopupMenu)不必要的{...Props},除非有什么情况是我们需要响应的?

@picodoth picodoth requested review from jljsj33, afc163, benjycui and yesmeck and removed request for jljsj33 May 5, 2018 09:05
@yesmeck
Copy link
Member

yesmeck commented May 5, 2018

#135 还为了可以自己传任意的 props 给 li

@picodoth
Copy link
Contributor Author

picodoth commented May 5, 2018

@yesmeck 那我看看直接把用户自己传的props打包到一个object里最后再放出来好了

@picodoth picodoth changed the title fix: passing onclick through [WIP]fix: passing onclick through May 5, 2018
return (
<li {...props} {...mouseEvents} className={className} role="menuitem">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jljsj33 亲 我们这个地方一定要{...props}么?因为现在这个li是包着children的,所以{...props}肯定会有问题。照道理说菜单title的onClick事件就应该是展开或者关闭子菜单啊?

return (
// ESLint is not smart enough to know that the type of `children` was checked.
/* eslint-disable */
<DOMWrap
{...props}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个地方也是,感觉更加没有理由{...props}

Copy link
Member

Choose a reason for hiding this comment

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

@jljsj33 主要是这里

@jljsj33
Copy link
Member

jljsj33 commented May 6, 2018

onClick 事件跟 props 有什么关系??? inherit props 只不过顺带把 onClick 事件做了而已,,如果不需要 onClick 下带,把这里改回来就行了。

menu/src/SubPopupMenu.js

Lines 273 to 276 in 8c23c8c

onClick: (e) => {
(childProps.onClick || noop)(e);
this.onClick(e);
},

@picodoth
Copy link
Contributor Author

picodoth commented May 7, 2018

还是在#140处理吧!

@picodoth picodoth closed this May 7, 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