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.

Sign up for GitHub

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(Popover): popover ref.current.show() invalid #6732

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(Popover): popover ref.current.show() invalid #6732

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/components/popover/popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,17 @@ export const Popover = forwardRef<PopoverRef, PopoverProps>((p, ref) => {
useImperativeHandle(
ref,
() => ({
show: () => setVisible(true),
show: () => {
stateRef.current.isClickShow = true
setVisible(true)
},
hide: () => setVisible(false),
visible,
}),
[visible]
)

const stateRef = useRef({ isClickShow: false })
const targetRef = useRef<Wrapper>(null)
const floatingRef = useRef<HTMLDivElement>(null)
const arrowRef = useRef<HTMLDivElement>(null)
Expand Down Expand Up @@ -196,6 +200,10 @@ export const Popover = forwardRef<PopoverRef, PopoverProps>((p, ref) => {
useClickAway(
() => {
if (!props.trigger) return
if (stateRef.current.isClickShow) {
stateRef.current.isClickShow = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个实现方式有个问题，如果用户点击了一个阻止冒泡的元素且执行了ref.current.show()。会导致stateRef.current.isClickShow为true的状态持续在Popover组件内，下次点击任意地方都不会隐藏。

Copy link
Collaborator

Choose a reason for hiding this comment

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

我尝试在stateRef.current.isClickShow = true 后，加一个定时器及时修改为false，可以解决该问题。但我不太确定这样的解决方案是否有其他副作用。

useImperativeHandle(
    ref,
    () => ({
      show: () => {
        stateRef.current.isClickShow = true
        setTimeout(() => {
          stateRef.current.isClickShow = false
        })
        setVisible(true)
      },
      hide: () => setVisible(false),
      visible,
    }),
    [visible]
  )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

确实会有这个问题 那不使用isClickShow了 直接改成
show: () => {
setTimeout(() => {
setVisible(true)
})
},
是不是更直接点

Copy link
Collaborator

Choose a reason for hiding this comment

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

确实会有这个问题 那不使用isClickShow了 直接改成 show: () => { setTimeout(() => { setVisible(true) }) }, 是不是更直接点

直接使用定时器会造成break change。不满足之前的部分测试用例。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我看之前好像没有写过关于通过ref打开popver的测试用例

Copy link
Collaborator

Choose a reason for hiding this comment

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

不好意思，我记错了..
Popover测试用例确实没有ref相关的，欢迎pr补充。

Copy link
Contributor Author

@DaJianWu DaJianWu Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

我想了下，在不改变组件现有代码结构的设计的前提下，可以通过以下几种方式：
show方法直接通过setTimeout延时执行setVisible
通过isClickShow来标识show方法调用并在setTimeout重置isClickShow的状态
show方法要求外部传递evnet参数来阻止冒泡
感觉都可以实现，但又感觉都不是最优解，大佬有什么好的建议吗

Copy link
Collaborator

Choose a reason for hiding this comment

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

我想了下，在不改变组件现有代码结构的设计的前提下，可以通过以下几种方式： show方法直接通过setTimeout延时执行setVisible 通过isClickShow来标识show方法调用并在setTimeout重置isClickShow的状态 show要求外部传递evnet参数来组织冒泡 感觉都可以实现，但又感觉都不是最优解，大佬有什么好的建议吗

@zombieJ 豆酱大佬，有空看下。以上方案我个人觉得都不够优雅。我的建议是官网补充demo与Q&A，针对 #6731 遇到的问题给出相应解决方案。如自行增加setTimeout或e.stopPropagation()。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

等确定方案了我把这块的测试用例补充一下

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个还有后续吗？

return
}
setVisible(false)
},
[() => targetRef.current?.element, floatingRef],
Expand Down
Loading