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(TimePicker): fix the 'modelValue' watch defect #13095

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

Conversation

pany-ang
Copy link
Contributor

解决该 issue 中提到的第 2 个问题: #13084

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.68%. Comparing base (867e612) to head (51d89eb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13095      +/-   ##
==========================================
+ Coverage   89.66%   89.68%   +0.01%     
==========================================
  Files         257      257              
  Lines        6987     6988       +1     
  Branches     1724     1725       +1     
==========================================
+ Hits         6265     6267       +2     
+ Misses        382      380       -2     
- Partials      340      341       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pany-ang
Copy link
Contributor Author

同理,逻辑相似的 DatePicker 组件也有该问题。如果这个 PR 通过了,到时候我会再提交一个修复 DatePicker 的 PR

@inottn
Copy link
Collaborator

inottn commented Aug 30, 2024

可以补充一下单元测试

@pany-ang
Copy link
Contributor Author

该 PR 不太好用单测来测试,暂时没找到通过单测触发新增逻辑的办法。参考 issue 中提到的手动复现办法,也是需要两个组件 + 人工按照一定逻辑操作 4 次界面。

如果 CR 能通过,也许这里并不需要单测也行。

const _newValues = formatValueRange(newValues, columns.value);
if (
!isSameValue(_newValues, currentValues.value) ||
!isSameValue(_newValues, newValues)
Copy link
Member

Choose a reason for hiding this comment

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

这里为什么需要加这个判断呢,从 issue 中没有看到解释

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. v-model 修改为 ['00', '00'] 时,此时 UI 上没有 ['00', '00'] 选项,UI 被迫选中最接近的一个值,但是 v-model 没有同步更新为该值,依旧是 ['00', '00'](UI 变了,但 v-model 没变)(可能是一个 BUG)

对应的是 issue 中这段话

Copy link
Member

Choose a reason for hiding this comment

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

对于新加的这个 if 判断,似乎只有在 currentValues.value 和 _newValues 相等的时候会触发,那下面的 currentValues.value = _newValues; 赋值有什么意义呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

该赋值为了触发这段侦听器:

    watch(currentValues, (newValues) => {
      if (!isSameValue(newValues, props.modelValue)) {
        emit('update:modelValue', newValues);
      }
    });

目的是让内部的 currentValues 同步到外部的 v-model 绑定的变量

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

Successfully merging this pull request may close these issues.

4 participants