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

fix: Menu[onClick] should not be triggered by propagated onClick event #140

Merged
merged 3 commits into from
May 7, 2018

Conversation

benjycui
Copy link
Member

@benjycui benjycui commented May 5, 2018

No description provided.

@benjycui
Copy link
Member Author

benjycui commented May 5, 2018

修复:点击菜单项后,Menu[onClick] 被触发两次,第一次的参数与 README.md 的一致,第二次的参数为 React 的 event 对象。

@@ -77,7 +77,7 @@ const nestSubMenu = (<SubMenu title={<span>offset sub menu 2</span>} key="4" pop
function onOpenChange(value) {
console.log('onOpenChange', value);
}
const commonMenu = (<Menu onSelect={handleSelect} onOpenChange={onOpenChange}>
const commonMenu = (<Menu onClick={handleClick} onOpenChange={onOpenChange}>
Copy link
Member Author

Choose a reason for hiding this comment

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

重现 bug。

};
}
// In case that onClick/onMouseLeave/onMouseEnter is passed down from owner
const mouseEvent = {
Copy link
Member Author

Choose a reason for hiding this comment

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

onClick 会被透传下来,所以 disabled 时需要显式设置为空。

@benjycui
Copy link
Member Author

benjycui commented May 5, 2018

语雀这边先锁版本吧,感觉这个点击行为需要讨论一下。

  1. 点击 Menu.Item,会先触发一次 onClick,然后冒泡,再依次触发 SubMenu 和 Menu 的 onClick
  2. 如果阻止冒泡,那么当 Menu 在 Dropdown 里面就无法触发 Dropdown 的 close 行为了
  3. 如果直接禁止 SubMenu 的 onClick 行为,就会导致单测里面的情况挂掉

只能在 SubMenu 里面判断 onClick 的类型解决?

@picodoth
Copy link
Contributor

picodoth commented May 5, 2018

Looking

@picodoth
Copy link
Contributor

picodoth commented May 5, 2018

这个问题是这里引进的:https://github.com/react-component/menu/pull/135/files#diff-6288f5e4c6c3e87cf12a0727d10e3be3R173, 我看看有没有更合理的fix

@yesmeck
Copy link
Member

yesmeck commented May 5, 2018

#135

这个 PR 要重新实现下。

@picodoth
Copy link
Contributor

picodoth commented May 5, 2018

可能有一个合理的fix, 现在出去一下,晚上继续搞

@picodoth
Copy link
Contributor

picodoth commented May 7, 2018

@benjycui 我觉得这样的处理是可行的,subMenu的onClick是加在title上的,单机title正常情况下行为应该就是内置的吧(展开子菜单啥的)。然后挂掉的单测的目的就是测试onClick事件被激发的次数,而且是上次刚加上去的,我觉的应该没问题。

只要能保证其他的props正常传到了subMenu以及menuItem,然后menuItem可以响应自定义的onClick事件,我觉得就是okay的。

@benjycui @afc163 @yesmeck @jljsj33

@picodoth
Copy link
Contributor

picodoth commented May 7, 2018

@benjycui 我试了下,rc-select的单测以及antD单测用这个分支跑都没问题,把这里挂掉的单测清掉我觉得就可以合了。。

@picodoth picodoth merged commit 7bd1e63 into master May 7, 2018
@picodoth picodoth deleted the fix-click branch May 7, 2018 06:39
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