-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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(select): filter break bug #756
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/tusimple/naive-ui/BjAqwKCjsWSEKgusnY3magombfoK |
Codecov Report
@@ Coverage Diff @@
## main #756 +/- ##
==========================================
- Coverage 40.39% 39.58% -0.81%
==========================================
Files 507 507
Lines 12396 12374 -22
Branches 3485 3467 -18
==========================================
- Hits 5007 4898 -109
- Misses 6476 6567 +91
+ Partials 913 909 -4
Continue to review full report at Codecov.
|
src/select/src/Select.tsx
Outdated
@@ -410,6 +410,7 @@ export default defineComponent({ | |||
function handleTriggerFocus (e: FocusEvent): void { | |||
doFocus(e) | |||
focusedRef.value = true | |||
handleTriggerClick() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这是个啥该法又,我觉得不行吧?这不是用 tab 键就能打开 menu 了?这和预期行为不一样
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
得找到出问题的根本原因
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onClick得换成onMousedown, onClick触发机制和focus的触发机制不一样,所以造成了focus了,但是click没触发
src/select/src/Select.tsx
Outdated
@@ -713,7 +713,7 @@ export default defineComponent({ | |||
} | |||
loading={this.loading} | |||
focused={this.focused} | |||
onClick={this.handleTriggerClick} | |||
onMousedown={this.handleTriggerClick} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这块处理不能这么简单,得理一下逻辑,mousedown 干的事和 click 应该是有一些区别的。
如果有必要的话需要重新分配一下。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mousedown 可以触发 openMenu,focus 不能。
要注意各种 filter + multiple 的组合不要出问题,以及按 tab focus 的状态不要受影响。
@@ -209,6 +210,7 @@ export default defineComponent({ | |||
hoverRef.value = false | |||
} | |||
function handleMouseDown (e: MouseEvent): void { | |||
props.onMousedown?.(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不是每一个 mousedown 都要和 click 走一样的逻辑,这块的处理感觉不太行。
src/select/src/Select.tsx
Outdated
if (!props.filterable) { | ||
// already focused, don't need to return focus | ||
closeMenu() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
有什么情况会走到这个分支吗?不 filterable 是怎么会出现 patternInput 事件的
close #510
我先提一下,我不太清楚这么改会不会有其他问题,先让 @07akioni 看看