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 & DatePicker): fix the 'formatValueRange' function defect #13094

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pany-ang
Copy link
Contributor

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

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.66%. Comparing base (867e612) to head (9926b8f).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
packages/vant/src/date-picker/utils.ts 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13094      +/-   ##
==========================================
- Coverage   89.66%   89.66%   -0.01%     
==========================================
  Files         257      257              
  Lines        6987     6994       +7     
  Branches     1724     1726       +2     
==========================================
+ Hits         6265     6271       +6     
+ Misses        382      380       -2     
- Partials      340      343       +3     

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

@pany-ang
Copy link
Contributor Author

我试着看了一下相关组件,其中 Picker 组件内部有一个 watch:

watch(
      () => props.modelValue,
      (newValues) => {
        if (
          !isSameValue(newValues, selectedValues.value) &&
          !isSameValue(newValues, lastEmittedModelValue)
        ) {
          selectedValues.value = newValues.slice(0);
          lastEmittedModelValue = newValues.slice(0);
        }
      },
      { deep: true },
    );

如果将其中的 !isSameValue(newValues, lastEmittedModelValue) 移除就能解决第 1 点所说的 BUG,但这是在我不熟悉该组件的情况下找到的修复办法,所以移除这行代码可能会引起其他问题。

牵扯的组件和逻辑有点复杂...要是有空我可能会试着再看看,或者等待其他人来修复这个问题

相比起移除 issue 里提到的 !isSameValue(newValues, lastEmittedModelValue) 语句,我更倾向于将 TimePicker 组件用到的 formatValueRange 方法增加一个兜底逻辑:

export const formatValueRange = (
  values: string[],
  columns: PickerOption[][],
) => {
  if (values.length === 0) {
    return columns.map((column) => column[0]?.value?.toString() ?? ''); // 新增的兜底逻辑
  }
  return values.map((value, index) => {
    const column = columns[index];
    if (column.length) {
      const minValue = +column[0].value!;
      const maxValue = +column[column.length - 1].value!;
      return padZero(clamp(+value, minValue, maxValue));
    }
    return value;
  });
};

当 v-model 入参为 undefined,内部的私有变量将启用默认值 [],当该私有变量为 [] 时,新增的兜底逻辑将生效,将该变量修改为每一列可选数据的第一项。

@pany-ang
Copy link
Contributor Author

说实话,能修复这个 BUG 的地方似乎有点多。我其实一时分不清到底哪里才是真正应该修复的地方,TimePickerPicker 两个组件的数据交互因为大量的 watch 的存在,实在是有点绕。

我没有将该 PR 为什么能解决问题解释的很清楚,因为它实在是不好解释。这可能需要先去仔细查看对应 issue 中的内容,然后仔细体验一下最小复现。

@inottn
Copy link
Collaborator

inottn commented Aug 30, 2024

可以补充一下单元测试

@pany-ang pany-ang changed the title fix(TimePicker): fix the 'formatValueRange' function defect fix(TimePicker & DatePicker): fix the 'formatValueRange' function defect Aug 31, 2024
@pany-ang
Copy link
Contributor Author

单测已添加。但是单测只是单纯的验证了一下新增的逻辑是否生效,并没有去验证是否解决了 issue 中的 BUG,因为我没想到该如何编写这种复杂场景的单测

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.

3 participants