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: useWave unref #7108

Merged
merged 4 commits into from
Dec 18, 2023
Merged

fix: useWave unref #7108

merged 4 commits into from
Dec 18, 2023

Conversation

puppet-666
Copy link
Contributor

close #7106

Verified

This commit was signed with the committer’s verified signature.
beeme1mr Michael Beemer
@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

感觉不是这样去修

Copy link
Member

@kovsu kovsu left a comment

Choose a reason for hiding this comment

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

这里不用改

@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

https://github.com/vueComponent/ant-design-vue/blob/main/components/config-provider/hooks/useConfigInject.ts#L47C1-L47C1
把这里的 props.wave 改成 props.disabled 应该就行了,我再看一下其他地方的 wave 组件有没有问题

@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

main/components/config-provider/hooks/useConfigInject.ts#L47C1-L47C1 把这里的 props.wave 改成 props.disabled 应该就行了,我再看一下其他地方的 wave 组件有没有问题

应该只要改这里就好了,因为 props 里没有定义 wave,而且当 Wave 组件的 disabled 为 true 的时候,wave 也应该是 false,这是符合逻辑的。这个报错的原因是 props.wave 是 undefined,config-provider 里的 wave 也是 undefined 的问题导致的报错。

@puppet-666
Copy link
Contributor Author

这里改有不同的方式,主要这里不太清楚你 API 设计的初衷

@puppet-666
Copy link
Contributor Author

我一开始也是想改这里的默认值

@puppet-666
Copy link
Contributor Author

main/components/config-provider/hooks/useConfigInject.ts#L47C1-L47C1 把这里的 props.wave 改成 props.disabled 应该就行了,我再看一下其他地方的 wave 组件有没有问题

应该只要改这里就好了,因为 props 里没有定义 wave,而且当 Wave 组件的 disabled 为 true 的时候,wave 也应该是 false,这是符合逻辑的。这个报错的原因是 props.wave 是 undefined,config-provider 里的 wave 也是 undefined 的问题导致的报错。

这里改成 props,wave.disabled 也会有问题吧,默认不配置 props.wave 不也还是会走 config-provider 里的 wave ?

@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

@puppet-666 props.wave 改成 props.disabled,不是 props.wave.disabled 😅

@puppet-666
Copy link
Contributor Author

@puppet-666 props.wave 改成 props.disabled,不是 props.wave.disabled 😅

wave 返回 {disabled?: boolean;} 这里改成 props.disabled 的话,让 showWave 里面 wave.value?.disabled 强行 undefined ? 好像有点奇怪

@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

@puppet-666 也对,我的问题,那就是 { disabled: props.disabled } 这样,我之前会给 wave 写成对象是为了后面可能还要加属性考虑
好像也不对,想想怎么改,反正就是操作 props.disabled 属性而不是 props.wave

@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

const wave = computed<{
    disabled?: boolean;
  }>(() => props.disabled ? { disabled: props.disabled } : configProvider.wave.value);

这样?你觉得呢

@puppet-666
Copy link
Contributor Author

const wave = computed<{
    disabled?: boolean;
  }>(() => props.disabled ? { disabled: props.disabled } : configProvider.wave.value);

这样?你觉得呢

我这边改下

@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

我有一个想法,就是 wave 的取值 只靠 config provider。然后如果是 disabled 的话就不去运行 showWave 这个方法

@puppet-666
Copy link
Contributor Author

我有一个想法,就是 wave 的取值 只靠 config provider。然后如果是 disabled 的话就不去运行 showWave 这个方法

这样config provider需要有默认值,我不确定这里是否会和antd产生分歧。但这样确实比上面根据disabled判断来的直接,使用disabled无形间感觉产生了优先级概念,而这个disabled的状态我看着好像文档上没看到有,有点理解上的冲突

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

@puppet-666 如果 是 disabled 的话,都不会运行 showWave。所以感觉在里面判断 props.disabled 没有什么意义。

@puppet-666
Copy link
Contributor Author

😂 看起来对于这里的修复方案还是有待商榷,或者这里是否需要团队里其他人的意见?还是你这边发起一个新 PR ?@kovsu

@kovsu
Copy link
Member

kovsu commented Nov 14, 2023

想了想,按你第一次提交来吧,然后麻烦改一下 props.wave 那个地方,我觉得直接删除 props.wave 就靠 config-provider 去判断吧。😂

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@puppet-666
Copy link
Contributor Author

想了想,按你第一次提交来吧,然后麻烦改一下 props.wave 那个地方,我觉得直接删除 props.wave 就靠 config-provider 去判断吧。😂

提交了,另外还有一个文档的pr辛苦也看下 #7107

@ramwin ramwin mentioned this pull request Nov 19, 2023
1 task
@kovsu kovsu mentioned this pull request Nov 24, 2023
1 task
@tangjinzhou tangjinzhou merged commit d140523 into vueComponent:main Dec 18, 2023
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

【Button】click 事件触发wave组件showWave()方法报错
3 participants