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(select): filter break bug #756

Merged
merged 5 commits into from
Aug 7, 2021
Merged

fix(select): filter break bug #756

merged 5 commits into from
Aug 7, 2021

Conversation

Volankey
Copy link
Collaborator

@Volankey Volankey commented Aug 2, 2021

close #510

我先提一下,我不太清楚这么改会不会有其他问题,先让 @07akioni 看看

@vercel
Copy link

vercel bot commented Aug 2, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/BjAqwKCjsWSEKgusnY3magombfoK
✅ Preview: https://naive-ui-git-fork-volankey-510-tusimple.vercel.app

@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #756 (bb1dfb5) into main (489a5b6) will decrease coverage by 0.80%.
The diff coverage is 0.00%.

❗ Current head bb1dfb5 differs from pull request most recent head 79e56d4. Consider uploading reports for the commit 79e56d4 to get more accurate results
Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/select/src/Select.tsx 33.70% <0.00%> (-0.13%) ⬇️
src/skeleton/styles/light.ts 0.00% <0.00%> (-100.00%) ⬇️
src/progress/src/MultipleCircle.tsx 0.00% <0.00%> (-86.67%) ⬇️
src/skeleton/src/Skeleton.tsx 0.00% <0.00%> (-82.76%) ⬇️
src/_internal/slot-machine/src/SlotMachine.tsx 0.00% <0.00%> (-67.75%) ⬇️
src/progress/src/Circle.tsx 9.09% <0.00%> (-54.55%) ⬇️
...c/_internal/slot-machine/src/SlotMachineNumber.tsx 0.00% <0.00%> (-37.50%) ⬇️
src/progress/src/Line.tsx 37.03% <0.00%> (-37.04%) ⬇️
src/skeleton/styles/dark.ts 0.00% <0.00%> (-33.34%) ⬇️
src/_utils/naive/attribute.ts 0.00% <0.00%> (-25.00%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56edac0...79e56d4. Read the comment docs.

@@ -410,6 +410,7 @@ export default defineComponent({
function handleTriggerFocus (e: FocusEvent): void {
doFocus(e)
focusedRef.value = true
handleTriggerClick()
Copy link
Collaborator

Choose a reason for hiding this comment

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

这是个啥该法又,我觉得不行吧?这不是用 tab 键就能打开 menu 了?这和预期行为不一样

Copy link
Collaborator

Choose a reason for hiding this comment

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

得找到出问题的根本原因

Copy link
Collaborator Author

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没触发

@@ -713,7 +713,7 @@ export default defineComponent({
}
loading={this.loading}
focused={this.focused}
onClick={this.handleTriggerClick}
onMousedown={this.handleTriggerClick}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这块处理不能这么简单,得理一下逻辑,mousedown 干的事和 click 应该是有一些区别的。

如果有必要的话需要重新分配一下。

Copy link
Collaborator

@07akioni 07akioni Aug 3, 2021

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

不是每一个 mousedown 都要和 click 走一样的逻辑,这块的处理感觉不太行。

Comment on lines 515 to 518
if (!props.filterable) {
// already focused, don't need to return focus
closeMenu()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

有什么情况会走到这个分支吗?不 filterable 是怎么会出现 patternInput 事件的

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants