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(ColorPicker): fix on-complete parameter value is not updated in time #749

Merged
merged 5 commits into from
Aug 8, 2021
Merged

fix(ColorPicker): fix on-complete parameter value is not updated in time #749

merged 5 commits into from
Aug 8, 2021

Conversation

aisen60
Copy link
Contributor

@aisen60 aisen60 commented Aug 1, 2021

close #748

@vercel
Copy link

vercel bot commented Aug 1, 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/53NMmZrcTaPi5bWtqWZgmNuN76rJ
✅ Preview: https://naive-ui-git-fork-aisen60-fix-color-picker-tusimple.vercel.app

@aisen60 aisen60 changed the title Fix/color picker fix(ColorPicker): fix on-complete parameter value is not updated in time Aug 1, 2021
@@ -391,7 +391,7 @@ export default defineComponent({
}

function handleComplete (pushStack: boolean = true): void {
const { value } = mergedValueRef
const { value } = uncontrolledValueRef
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个修法有点问题,造成这个现象的核心原因是原来 Input 只对 onChange 做了处理,改这里觉得哪里不太对。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我觉得是组件 onUpdate:value 执行完后,mergedValueRef 还没来得及更新,就执行了 onComplete。我改一下😊

Copy link
Collaborator

Choose a reason for hiding this comment

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

我有点印象,可能和一个叫 upcomingValue 值的处理有关系。

设定那个值是因为我们要保证受控的情况下颜色光标不乱跳,因为一个值值对应一个位置,但是多个位置可以对应一个值,所以使用传统的写法光标就会跳。

跳光标的问题我在其他库里看见过,具体表现就是 HSL 的情况下没法让光标在底边上自由移动。

@@ -17,6 +17,7 @@
### Fixes

- 修复 `n-dropdown` 循环渲染时点击异常
- 修复 `n-color-picker` `on-complete` 回调参数 `value` 更新异常
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 修复 `n-color-picker` `on-complete` 回调参数 `value` 更新异常
- 修复 `n-color-picker` `on-complete` 回调参数 `value` 更新异常, 关闭 [#748](https://github.com/TuSimple/naive-ui/issues/748).

@@ -17,6 +17,7 @@
### Fixes

- Fix `n-dropdown` click exception when using v-for.
- Fix `n-color-picker` `on-complete` callback parameter `value` update error. closes [#748](https://github.com/TuSimple/naive-ui/issues/748).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fix `n-color-picker` `on-complete` callback parameter `value` update error. closes [#748](https://github.com/TuSimple/naive-ui/issues/748).
- Fix `n-color-picker` `on-complete` callback parameter `value` update error, closes [#748](https://github.com/TuSimple/naive-ui/issues/748).

Comment on lines 390 to 392
setTimeout(() => {
handleComplete()
}, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

为什么加上 setTimeout 就好了,有发现吗?

是不是 nextTick 就行?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

为什么加上 setTimeout 就好了,有发现吗?

是不是 nextTick 就行?

@07akioni

不好意思,太忙了,现在才回复。

是的,nextTick 就可以,不需要 setTimeout。

当输入框修改完毕后并且失去焦点时,on-complete 的回调参数依旧是上一个旧的值。出现这个原因是因为 on-complete 的回调参数 value 获取的是 mergedValueRef 这个值中的 value,而 mergedValueRef 中的 value 是通过 props 传进来的。在执行 on-complete 之前,会先执行 on-update:value,这时候 props.value 已经更新了,但 mergedValueRef 还没来得及更新,所以,on-complete 的回调参数 value 取得是上一个 mergedValueRef 中的 value。

https://github.com/TuSimple/naive-ui/blob/018688cef19ab407a50eb67bc2c83783ba3d83dd/src/color-picker/src/ColorPicker.tsx#L155-L158

https://github.com/TuSimple/naive-ui/blob/018688cef19ab407a50eb67bc2c83783ba3d83dd/src/color-picker/src/ColorPicker.tsx#L388-L391

https://github.com/TuSimple/naive-ui/blob/018688cef19ab407a50eb67bc2c83783ba3d83dd/src/color-picker/src/ColorPicker.tsx#L393-L400

@07akioni 07akioni merged commit 91942e7 into tusen-ai:main Aug 8, 2021
07akioni added a commit that referenced this pull request Aug 8, 2021
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.

Color Picker component input update error
3 participants