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

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

Conversation

DaJianWu
Copy link
Contributor

@DaJianWu DaJianWu commented Aug 29, 2024

Copy link
Contributor

PR preview has been successfully built and deployed to https://antd-mobile-preview-pr-6732.surge.sh

@zombieJ
Copy link
Member

zombieJ commented Aug 29, 2024

CI failed. Pls check

@@ -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

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.

这个还有后续吗?

@xiaoyao96
Copy link
Collaborator

xiaoyao96 commented Aug 30, 2024 via email

@xiaoyao96 xiaoyao96 requested a review from zombieJ August 30, 2024 05:59
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