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(range): usememo #2638

Merged
merged 5 commits into from
Oct 12, 2024
Merged

fix(range): usememo #2638

merged 5 commits into from
Oct 12, 2024

Conversation

xiaoyatong
Copy link
Collaborator

@xiaoyatong xiaoyatong commented Oct 11, 2024

🤔 这个变动的性质是?

  • 新特性提交
  • 日常 bug 修复
  • 站点、文档改进
  • 演示代码改进
  • 组件样式/交互改进
  • TypeScript 定义更新
  • 包体积优化
  • 性能优化
  • 功能增强
  • 国际化改进
  • 重构
  • 代码风格优化
  • 测试用例
  • 分支合并
  • 其他改动(是关于什么的改动?)

🔗 相关 Issue

💡 需求背景和解决方案

☑️ 请求合并前的自查清单

⚠️ 请自检并全部勾选全部选项⚠️

  • 文档已补充或无须补充
  • 代码演示已提供或无须提供
  • TypeScript 定义已补充或无须补充
  • fork仓库代码是否为最新避免文件冲突
  • Files changed 没有 package.json lock 等无关文件

Summary by CodeRabbit

  • 新特性

    • 更新了范围组件的默认值和最大值,增强了用户体验。
    • 增加了多个用户交互的测试用例,确保范围组件在不同情况下的行为正确。
    • 改进了范围组件的响应式设计,支持更好的垂直和RTL布局。
    • 简化了范围组件的逻辑,提高了可读性。
  • Bug 修复

    • 优化了范围计算逻辑,避免了潜在的除零错误。
  • 样式

    • 增加了最大宽度和高度属性,提升了组件的视觉效果和适应性。

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 77.27273% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.85%. Comparing base (5b21eca) to head (affec60).
Report is 2 commits behind head on next.

Files with missing lines Patch % Lines
src/packages/range/range.tsx 77.27% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2638      +/-   ##
==========================================
+ Coverage   82.98%   83.85%   +0.86%     
==========================================
  Files         219      218       -1     
  Lines       17909    17887      -22     
  Branches     2550     2608      +58     
==========================================
+ Hits        14862    14999     +137     
+ Misses       3042     2883     -159     
  Partials        5        5              

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

Copy link

coderabbitai bot commented Oct 11, 2024

Walkthrough

此次更改涉及对Range组件的多个方面进行修改,包括属性的更新、样式的增强以及组件逻辑的优化。在demo5.tsx中,defaultValuemax属性的值被调整,以改变范围滑块的初始状态和上限。在样式文件range.scss中,新增了响应式的max-widthmax-height属性,并对垂直和RTL布局进行了样式调整。此外,range.taro.tsxrange.tsx中的组件逻辑进行了重构,以提高性能和可读性。

Changes

文件路径 更改摘要
src/packages/range/demos/h5/demo5.tsx 更新Range组件的defaultValue0改为-8max值从10改为-6min值保持不变。
`src/packages/range/range.scss .nut-range-bar类新增max-widthmax-height属性,设置为100%,并调整.nut-range类的样式以适应垂直和RTL布局。
`src/packages/range/range.taro.tsx 移除useCallback钩子,改用useMemo计算scope,简化markClassName函数,调整onChangeonStartonEnd属性的处理,重构按钮渲染逻辑。
`src/packages/range/range.tsx 添加useMemo优化scope的计算,更新calcMainAxiscalcOffset函数以直接引用scope,简化onTouchMove函数,调整按钮渲染以移除多余检查。

Possibly related PRs

Suggested reviewers

  • oasis-cloud

Poem

🐰 在范围内跳跃,滑块轻松调,
属性更新,样式更美好。
逻辑重构,性能提升,
兔子欢呼,庆祝新生!
让我们一起,快乐无穷! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (2)
src/packages/range/range.tsx (1)

182-182: 建议为新增代码添加单元测试

静态分析工具指出,第 182、229、269、274、326 行的代码未被测试覆盖。为了提高代码的可靠性和稳定性,建议为这些逻辑添加相应的单元测试。

您是否需要我协助编写这些单元测试,或者为此创建一个 GitHub Issue 来跟踪这个任务?

Also applies to: 229-229, 269-269, 274-274, 326-326

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 182-182: src/packages/range/range.tsx#L182
Added line #L182 was not covered by tests

src/packages/range/range.taro.tsx (1)

323-328: 事件处理函数命名可能引起混淆

renderRangeButton 中,内部事件处理函数名称与组件 props 中的回调函数 onTouchStart 重名,可能导致混淆。

建议将内部事件处理函数重命名,例如改为 handleTouchStart,以区别于 props 中的回调函数。

- onTouchStart={(e: any) => {
+ handleTouchStart={(e: any) => {
    setButtonIndex(index)
    onTouchStart(e)
  }}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5b21eca and d993c39.

📒 Files selected for processing (4)
  • src/packages/range/demos/h5/demo5.tsx (1 hunks)
  • src/packages/range/range.scss (1 hunks)
  • src/packages/range/range.taro.tsx (8 hunks)
  • src/packages/range/range.tsx (7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/packages/range/range.tsx

[warning] 182-182: src/packages/range/range.tsx#L182
Added line #L182 was not covered by tests


[warning] 229-229: src/packages/range/range.tsx#L229
Added line #L229 was not covered by tests


[warning] 269-269: src/packages/range/range.tsx#L269
Added line #L269 was not covered by tests


[warning] 274-274: src/packages/range/range.tsx#L274
Added line #L274 was not covered by tests


[warning] 326-326: src/packages/range/range.tsx#L326
Added line #L326 was not covered by tests

🔇 Additional comments (5)
src/packages/range/demos/h5/demo5.tsx (1)

11-12: 请考虑这些更改对用户体验的影响

这些更改缩小了滑块的范围并调整了默认值:

  1. 默认值从0改为-8,这可能会影响用户的初始体验。
  2. 最大值从10降至-6,大幅缩小了可选范围。

建议:

  1. 在组件文档中清楚地说明这些更改的原因。
  2. 考虑添加注释解释为什么选择这些特定的值。
  3. 确保这些更改与组件的预期用途一致。

运行以下脚本以验证这些更改是否与其他演示或测试保持一致:

src/packages/range/range.scss (1)

37-38: LGTM! 增强了组件的响应性

这些更改通过添加 max-widthmax-height 属性来限制范围条的尺寸,确保它不会超出其父容器的尺寸。这种修改提高了范围组件的响应性,使其在不同大小的容器中都能正确显示。

src/packages/range/range.tsx (1)

7-7: useMemo 的使用优化了性能

引入 useMemo 来计算 scope,有效避免了每次渲染时的重复计算,提升了组件的性能表现。

Also applies to: 147-149

src/packages/range/range.taro.tsx (2)

178-182: marksStyle 计算可能不准确

scope 非常小或为 0.1 时,(mark - min) / scope 的结果可能过大,导致标记位置渲染错误。

请确认在 max === min 的情况下,marksStyle 函数能正确计算标记位置。


146-146: ⚠️ Potential issue

变量名拼写错误

setEaxctValue 应为 setExactValue,请修正拼写以避免潜在的问题。

应用以下修改以纠正拼写错误:

- const [exactValue, setEaxctValue] = useState<RangeValue>(
+ const [exactValue, setExactValue] = useState<RangeValue>(

Likely invalid or redundant comment.

@@ -285,12 +266,12 @@
const rect = getRect(root.current)
let delta = touch.deltaX.current
let total = rect.width
let diff = (delta / total) * scope()
let diff = (delta / total) * scope
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

提取重复的计算逻辑以提高代码可读性

在代码中,diff = (delta / total) * scope 的计算出现了多次。建议将此计算提取为一个函数或常量,以遵循 DRY(Don't Repeat Yourself)原则,提升代码的可维护性和清晰度。

可以考虑如下修改:

+ const calculateDiff = () => (delta / total) * scope

- let diff = (delta / total) * scope
+ let diff = calculateDiff()

Also applies to: 274-274

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 269-269: src/packages/range/range.tsx#L269
Added line #L269 was not covered by tests

Comment on lines 147 to 149
const scope = useMemo(() => {
return max === min ? 0.1 : max - min
}, [max, min])
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议更妥善地处理 max 等于 min 的情况

当前,当 max === min 时,scope 被设置为 0.1,这可能会导致计算结果不精确或产生意料之外的行为。建议在这种情况下,直接禁用滑块或给出提示,而不是使用固定的 0.1 作为范围。

您可以考虑如下修改:

const scope = useMemo(() => {
  if (max === min) {
+   // 当 max 等于 min 时,处理逻辑
+   return 1 // 或根据需求返回适当的值
  }
  return max - min
}, [max, min])

Committable suggestion was skipped due to low confidence.

@@ -250,7 +226,7 @@ export const Range: FunctionComponent<
delta = (event.detail.y ? event.detail.y : event.clientY) - rect.top
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

状态更新函数拼写错误

在调用状态更新函数时,setEaxctValue 应为 setExactValue

请应用以下修正:

- setEaxctValue(current)
+ setExactValue(current)

以及

- setEaxctValue(newValue)
+ setExactValue(newValue)

Also applies to: 272-272

Comment on lines 148 to 149
return max === min ? 0.1 : max - min
}, [max, min])
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

scope 的处理可能导致问题

max === min 时,将 scope 设置为 0.1 可能会引起计算上的不精确,导致滑块位置计算错误。

建议在 max === min 的情况下,直接将滑块设置在起始位置,或者提示用户 maxmin 不应相等,以避免计算错误。

Comment on lines +131 to +137
let upperBound = max
if (range && Array.isArray(current)) {
lowerBound = current[0]
upperBound = current[1]
} else {
upperBound = current as number
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

单值模式下的逻辑可能有误

markClassName 函数中,当 rangefalse 时,只更新了 upperBoundlowerBound 仍保持为 min。这可能导致 isActive 判断不准确。

建议在单值模式下,将 lowerBoundupperBound 都设置为当前值,以确保标记的激活状态正确。

应用以下修改:

} else {
-   upperBound = current as number
+   lowerBound = current as number
+   upperBound = current as number
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let upperBound = max
if (range && Array.isArray(current)) {
lowerBound = current[0]
upperBound = current[1]
} else {
upperBound = current as number
}
let upperBound = max
if (range && Array.isArray(current)) {
lowerBound = current[0]
upperBound = current[1]
} else {
lowerBound = current as number
upperBound = current as number
}

Comment on lines 316 to +338
}
const renderRangeButton = () => {
return [0, 1].map((item, index) => {
const cls = `${index === 0 ? 'nut-range-button-wrapper-left' : ''}
${index === 1 ? 'nut-range-button-wrapper-right' : ''}`
return (
<div
key={index}
className={cls}
onTouchStart={(e: any) => {
setButtonIndex(index)
onTouchStart(e)
}}
onTouchMove={onTouchMove}
onTouchEnd={onTouchEnd}
onTouchCancel={onTouchEnd}
onClick={(e) => e.stopPropagation()}
>
{renderButton(index)}
</div>
)
})
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

优化 renderRangeButton 函数的返回

renderRangeButton 函数中,直接返回 map 的结果可以提高代码可读性。

可以简化为:

- return [0, 1].map((item, index) => {
+ return (
    [0, 1].map((item, index) => {
      // ...代码内容
    })
  )

Committable suggestion was skipped due to low confidence.

const isRange = (val: any) => {
return !!range && Array.isArray(val)
}

const scope = useMemo(() => {
return max === min ? 0.1 : max - min
Copy link
Collaborator

Choose a reason for hiding this comment

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

为什么设置 0.1 ?

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
src/packages/range/range.tsx (3)

147-152: 优化 scope 计算并改进错误处理

使用 useMemo 来计算 scope 是一个很好的优化。然而,有几点建议:

  1. 考虑使用更健壮的错误处理方式,而不是简单的 console.log。在生产环境中,这可能不是最佳选择。
  2. 为了国际化考虑,错误消息应该使用英语或支持多语言。
  3. max === min 时,可能需要特殊处理,而不是简单地继续计算 scope

建议修改如下:

const scope = useMemo(() => {
  if (max < min) {
    throw new Error('Max value must be greater than min value');
  }
  if (max === min) {
    return 1; // 或者返回一个小的默认值,具体取决于组件的需求
  }
  return max - min;
}, [max, min]);
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 149-150: src/packages/range/range.tsx#L149-L150
Added lines #L149 - L150 were not covered by tests


329-332: 优化 renderRangeButton 函数中的事件处理

renderRangeButton 函数中,将 setButtonIndex 调用移到 onTouchStart 处理程序内是一个很好的改进:

  1. 确保在触摸事件开始时正确设置按钮索引,这在逻辑上更合理。
  2. 可能修复了之前按钮索引可能未正确设置的潜在问题。

建议考虑使用 React 的 useCallback 来优化这些事件处理函数,特别是如果它们被传递给子组件时。例如:

const handleTouchStart = useCallback((index: number) => (e: TouchEvent<HTMLDivElement>) => {
  setButtonIndex(index);
  onTouchStart(e);
}, [onTouchStart]);

// 在 JSX 中使用
onTouchStart={(e) => handleTouchStart(index)(e)}

这可以进一步优化性能,特别是在复杂组件中。

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 329-329: src/packages/range/range.tsx#L329
Added line #L329 was not covered by tests


347-348: 优化 renderSingleButton 函数中的事件处理

renderSingleButton 函数中,直接将事件处理程序分配给 div 元素是一个很好的改进:

  1. 通过移除不必要的箭头函数,简化了代码。
  2. 通过避免在每次渲染时创建新的函数实例,可能会提高性能。

这个修改使单个按钮的渲染更加清晰和高效。然而,为了保持一致性,建议考虑使用与 renderRangeButton 相同的方法来处理这些事件,可能使用 useCallback 来进一步优化。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between d993c39 and 2a60c3b.

📒 Files selected for processing (2)
  • src/packages/range/range.taro.tsx (8 hunks)
  • src/packages/range/range.tsx (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/packages/range/range.taro.tsx
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/packages/range/range.tsx

[warning] 149-150: src/packages/range/range.tsx#L149-L150
Added lines #L149 - L150 were not covered by tests


[warning] 185-185: src/packages/range/range.tsx#L185
Added line #L185 was not covered by tests


[warning] 232-232: src/packages/range/range.tsx#L232
Added line #L232 was not covered by tests


[warning] 272-272: src/packages/range/range.tsx#L272
Added line #L272 was not covered by tests


[warning] 277-277: src/packages/range/range.tsx#L277
Added line #L277 was not covered by tests


[warning] 329-329: src/packages/range/range.tsx#L329
Added line #L329 was not covered by tests

🔇 Additional comments (5)
src/packages/range/range.tsx (5)

7-7: 导入 useMemo 以优化性能

引入 useMemo 是一个很好的做法,它可以帮助优化组件的性能,特别是在处理复杂计算时。这个改动为后续的性能优化铺平了道路。


155-157: 优化 calcMainAxis 和 calcOffset 函数

calcMainAxiscalcOffset 函数中直接使用 scope 值而不是调用它作为函数是一个很好的改进。这个变化:

  1. 与新的 memoized scope 值保持一致。
  2. 简化了代码。
  3. 通过避免不必要的函数调用,可能会提高性能。

这些修改使代码更加清晰和高效。

Also applies to: 161-161


232-232: 优化 click 函数中的值计算

click 函数中,value 的计算现在直接使用 scope,这是一个很好的改进:

  1. 保持了与新的 memoized scope 值的一致性。
  2. 简化了计算逻辑。
  3. 可能会带来轻微的性能提升。

这个修改使代码更加清晰和高效。

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 232-232: src/packages/range/range.tsx#L232
Added line #L232 was not covered by tests


272-272: 优化 onTouchMove 函数中的差值计算

onTouchMove 函数中,diff 的计算(包括水平和垂直情况)现在直接使用 scope,这是一个很好的改进:

  1. 与新的 memoized scope 值保持一致。
  2. 简化了计算逻辑。
  3. 可能带来轻微的性能提升。
  4. 保留了 RTL(从右到左)处理的逻辑。

这些修改使触摸移动的处理更加清晰和高效。

Also applies to: 277-277

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 272-272: src/packages/range/range.tsx#L272
Added line #L272 was not covered by tests


Line range hint 1-398: 总体评价:性能优化和代码改进

这次更改主要集中在性能优化和代码简化上,总体来说是很好的改进:

  1. 引入 useMemo 来计算 scope 是一个关键的优化,可以避免不必要的重复计算。
  2. 多个函数(如 calcMainAxiscalcOffsetclickonTouchMove)已更新为直接使用 memoized 的 scope 值,这简化了代码并可能提高性能。
  3. 事件处理逻辑在 renderRangeButtonrenderSingleButton 中得到了简化。

建议进一步改进的地方:

  1. 错误处理:考虑使用更健壮的错误处理机制,而不是简单的 console.log。
  2. 国际化:错误消息应该使用英语或支持多语言。
  3. 事件处理的一致性:考虑在 renderRangeButtonrenderSingleButton 中使用相同的事件处理方法,可能使用 useCallback 来进一步优化。

总的来说,这些更改提高了组件的性能和可读性,是一个很好的改进。

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 185-185: src/packages/range/range.tsx#L185
Added line #L185 was not covered by tests

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (4)
src/packages/range/__test__/range.spec.tsx (4)

164-172: 建议改进点击测试用例

这个测试用例模拟了对Range组件的点击,这是一个很好的开始。然而,目前缺少断言来验证点击后的预期行为。建议添加断言来检查点击后的状态变化,例如滑块的位置或值是否如预期更新。这将使测试更加完整和有意义。

示例改进:

test('range click test', () => {
  const { container } = render(
    <Range defaultValue={40} style={{ color: 'red' }} />
  )

  const rangeBar = container.querySelector('.nut-range-bar') as HTMLElement
  if (rangeBar) {
    fireEvent.click(rangeBar)
    // 添加断言,例如:
    // expect(container.querySelector('.number')?.innerHTML).not.toBe('40')
  }
})

184-192: 建议完善禁用状态的点击测试

这个测试用例正确地模拟了对禁用状态Range组件的点击,这是一个很好的测试场景。然而,目前缺少断言来验证点击在禁用状态下没有效果。建议添加断言来确保组件的状态在点击后没有变化。

示例改进:

test('range click disable test', () => {
  const initialValue = 40
  const { container } = render(
    <Range defaultValue={initialValue} disabled style={{ color: 'red' }} />
  )

  const rangeBar = container.querySelector('.nut-range-bar') as HTMLElement
  if (rangeBar) {
    fireEvent.click(rangeBar)
    // 添加断言,例如:
    // expect(container.querySelector('.number')?.innerHTML).toBe(initialValue.toString())
    // 确保滑块位置没有改变
    // const button = container.querySelector('.nut-range-button') as HTMLElement
    // expect(button.style.left).toBe(`${initialValue}%`)
  }
})

194-202: 建议完善垂直模式的点击测试

这个测试用例正确地模拟了对垂直模式Range组件的点击,这是一个重要的测试场景。然而,目前缺少断言来验证垂直模式下点击后的行为。垂直模式可能需要与水平模式不同的断言。

建议改进:

  1. 添加断言来检查点击后的状态变化。
  2. 特别关注垂直方向上的变化,如滑块的top属性而不是left属性。

示例改进:

test('range click vertical test', () => {
  const initialValue = 40
  const { container } = render(
    <Range defaultValue={initialValue} vertical style={{ color: 'red' }} />
  )

  const rangeBar = container.querySelector('.nut-range-bar') as HTMLElement
  if (rangeBar) {
    fireEvent.click(rangeBar)
    // 添加断言,例如:
    // expect(container.querySelector('.number')?.innerHTML).not.toBe(initialValue.toString())
    // 检查垂直方向上的变化
    // const button = container.querySelector('.nut-range-button') as HTMLElement
    // expect(button.style.top).not.toBe(`${initialValue}%`)
  }
})

204-220: 触摸测试结构良好,建议小幅改进

这个测试用例很好地模拟了Range组件的触摸交互,包括了touchStart、touchMove和touchEnd事件,这是一个全面的测试。测试结构清晰,并且包含了适当的断言,这些都是很好的实践。

然而,当前的断言假设触摸操作总是会将值设置为最大值(100)。为了使测试更加灵活和准确,建议根据模拟的触摸位置来计算预期值。

建议改进:

test('range touch test', () => {
  const initialValue = 40
  const { container } = render(<Range defaultValue={initialValue} />)

  const track = container.querySelector('.nut-range-button')
  const button = container.querySelector('.number')

  if (track) {
    const startX = 0
    const moveX = 10
    fireEvent.touchStart(track, {
      touches: [{ clientX: startX }],
    })
    fireEvent.touchMove(track, {
      touches: [{ clientX: moveX }],
    })
    fireEvent.touchEnd(track)
    
    // 计算预期值(这里需要根据实际的计算逻辑进行调整)
    const expectedValue = Math.min(100, initialValue + (moveX - startX))
    expect(button?.innerHTML).toBe(expectedValue.toString())
  }
})

这个改进将使测试更加准确,并且能够适应不同的触摸位置和移动距离。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2a60c3b and affec60.

⛔ Files ignored due to path filters (1)
  • src/packages/range/__test__/__snapshots__/range.spec.tsx.snap is excluded by !**/*.snap
📒 Files selected for processing (3)
  • src/packages/range/test/range.spec.tsx (2 hunks)
  • src/packages/range/range.taro.tsx (9 hunks)
  • src/packages/range/range.tsx (10 hunks)
🧰 Additional context used
🪛 Biome
src/packages/range/range.taro.tsx

[error] 264-266: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🪛 GitHub Check: codecov/patch
src/packages/range/range.tsx

[warning] 149-150: src/packages/range/range.tsx#L149-L150
Added lines #L149 - L150 were not covered by tests


[warning] 185-185: src/packages/range/range.tsx#L185
Added line #L185 was not covered by tests


[warning] 277-277: src/packages/range/range.tsx#L277
Added line #L277 was not covered by tests


[warning] 329-329: src/packages/range/range.tsx#L329
Added line #L329 was not covered by tests

🔇 Additional comments (12)
src/packages/range/__test__/range.spec.tsx (1)

3-3: 导入语句更新正确

导入 fireEvent 是一个好的做法,因为它为新添加的测试用例提供了模拟用户交互的能力。这个更改是必要的,并且遵循了只导入所需模块的最佳实践。

src/packages/range/range.tsx (9)

7-7: 导入 useMemo 以优化性能

引入 useMemo 是一个很好的做法,它可以帮助优化组件的性能,特别是在处理复杂计算时。这个改动为后续的性能优化铺平了道路。


92-94: 新增 exactValue 状态以跟踪精确值

引入 exactValue 状态是一个好主意,它可以帮助跟踪滑块的精确位置,特别是在拖动过程中。这样可以提高用户体验,使滑块移动更加流畅。

建议:考虑使用 useRef 而不是 useState 来存储 exactValue,因为它不需要触发重新渲染。这可能会稍微提高性能。

const exactValueRef = useRef<RangeValue>(value || defaultValue || 0);

147-152: 优化 scope 计算并增加错误处理

使用 useMemo 来计算 scope 是一个很好的优化。它可以避免在每次渲染时都重新计算这个值,特别是当 maxmin 不经常变化时。

对于 max < minmax === min 的情况进行检查也是一个很好的做法。然而,目前的处理方式可以进一步改进:

  1. 使用 console.warn 而不是 console.log 来输出警告信息。
  2. 在无效情况下返回一个默认值,以确保组件不会崩溃。

建议修改如下:

const scope = useMemo(() => {
  if (max <= min) {
    console.warn('Range 组件:max 的值需要大于 min 的值');
    return 1; // 返回一个默认值
  }
  return max - min;
}, [max, min]);

这样可以确保即使在无效输入的情况下,组件也能够正常工作。

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 149-150: src/packages/range/range.tsx#L149-L150
Added lines #L149 - L150 were not covered by tests


155-157: 优化 calcMainAxis 和 calcOffset 函数

scope 直接用作值而不是函数调用是一个很好的改进。这与新的 useMemo 实现保持一致,并且可能会略微提高性能。这种改变使代码更加清晰和高效。

Also applies to: 161-161


181-181: 优化 marksStyle 函数

marksStyle 函数中直接使用 scope 值而不是调用函数是一个很好的改进。这与之前的更改保持一致,使代码更加统一和高效。这种小的优化可以在大型应用中累积成显著的性能提升。

Also applies to: 185-185


232-233: 优化 click 函数并同步 exactValue

click 函数中直接使用 scope 值是一个很好的改进,与之前的更改保持一致。

添加 setExactValue(current) 是一个好主意,它确保了精确值与当前值同步。然而,考虑到 exactValue 的用途,我们可以进一步优化:

const newValue = min + (delta / total) * scope;
setExactValue(newValue);

这样可以直接设置 exactValue 为新计算的值,而不是当前值,可能会提供更精确的用户体验。


272-272: 优化 onTouchMove 函数并更新 exactValue

onTouchMove 函数中的更改非常好:

  1. 直接使用 scope 值保持了与之前更改的一致性,提高了代码的统一性。
  2. 添加 setExactValue(newValue) 确保了在触摸移动过程中精确值得到更新,这可以提供更流畅的用户体验。

这些更改使得滑块的行为在拖动过程中更加精确和响应迅速。

Also applies to: 277-277, 286-286


329-332: 优化事件处理器和按钮索引设置

这些更改提高了代码的效率和清晰度:

  1. renderRangeButton 中添加 setButtonIndex(index) 确保了在范围滑块上开始触摸时正确设置按钮索引。这对于正确处理多按钮滑块很重要。

  2. 直接引用事件处理器而不是包装在箭头函数中可以提高性能,因为它避免了在每次渲染时创建新的函数实例。

建议:考虑使用 useCallback 来记忆这些事件处理函数,特别是如果它们依赖于可能经常变化的 props 或 state。例如:

const handleTouchStart = useCallback((e: TouchEvent<HTMLDivElement>, index?: number) => {
  if (index !== undefined) setButtonIndex(index);
  onTouchStart(e);
}, [setButtonIndex, onTouchStart]);

这可以进一步优化性能,特别是在复杂组件中。

Also applies to: 347-348

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 329-329: src/packages/range/range.tsx#L329
Added line #L329 was not covered by tests


Line range hint 1-400: 总体评价:性能优化和用户体验提升

这次更新对 Range 组件进行了多方面的改进:

  1. 性能优化:引入 useMemo 来缓存 scope 计算,并在多处直接使用 scope 值而不是函数调用。
  2. 用户体验提升:引入 exactValue 状态来跟踪精确的滑块位置,使滑动更加流畅。
  3. 错误处理:增加了对无效 maxmin 值的检查。
  4. 代码清晰度:简化了多个计算函数,使代码更易读和维护。
  5. 事件处理优化:改进了事件处理器的绑定方式,潜在地提高了性能。

这些更改整体上遵循了 React 的最佳实践,提高了组件的质量和性能。虽然还有一些小的改进空间(如使用 useCallback 优化事件处理器),但总的来说,这是一次成功的重构。

建议:

  1. 考虑添加单元测试来覆盖新增的逻辑,特别是 exactValue 的处理和边界条件检查。
  2. 更新组件文档,反映新的 exactValue 行为和改进的性能特性。
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 185-185: src/packages/range/range.tsx#L185
Added line #L185 was not covered by tests

src/packages/range/range.taro.tsx (2)

128-143: 注意单值模式下 markClassName 函数的逻辑

在非范围选择模式下,markClassName 函数中 lowerBound 仍然等于 min。这可能导致对于标记的激活状态判断不准确。


320-341: 优化 renderRangeButton 函数的返回结构

renderRangeButton 函数中,可以直接返回 map 方法的结果,提高代码的简洁性和可读性。

Comment on lines +174 to +182
test('range click test', () => {
const state = {
value0: [30, 60],
}
const { container } = render(<Range range defaultValue={state.value0} />)
if (container.querySelector('.nut-range-bar')) {
fireEvent.click(container.querySelector('.nut-range-bar') as HTMLElement)
}
})
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

重复的测试名称和缺少断言

  1. 这个测试用例与前一个同名,可能会导致混淆。建议使用更具描述性的名称,如 'range click test in range mode'

  2. 同样,这个测试也缺少断言来验证点击后的行为。建议添加断言来检查点击后的状态变化。

示例改进:

test('range click test in range mode', () => {
  const initialValue = [30, 60]
  const { container } = render(<Range range defaultValue={initialValue} />)
  
  const rangeBar = container.querySelector('.nut-range-bar') as HTMLElement
  if (rangeBar) {
    fireEvent.click(rangeBar)
    // 添加断言,例如:
    // const newValue = container.querySelectorAll('.number')
    // expect(newValue[0].innerHTML).not.toBe(initialValue[0].toString())
    // expect(newValue[1].innerHTML).not.toBe(initialValue[1].toString())
  }
})

Comment on lines +155 to +157
return isRange(modelVal)
? `${((modelVal[1] - modelVal[0]) * 100) / scope}%`
: `${((modelVal - min) * 100) / scope}%`
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

确保 scope 不为零以避免计算错误

在函数 calcMainAxiscalcOffsetmarksStyleonTouchMove 等多处,使用了 scope 进行除法运算。如果 scope 为零,可能导致计算结果为 InfinityNaN。应在使用前验证 scope 的值,确保计算的可靠性。

建议在计算之前检查 scope 值:

if (scope === 0) {
  // 处理 scope 为零的情况
  return;
}

Also applies to: 161-161, 181-181, 185-185, 232-232, 272-272, 277-277

const onTouchStart = (event: any) => {
if (disabled) {
return
}
touch.start(event)
setEaxctValue(current)
setExactValue(current)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

检查 exactValue 状态的必要性

onTouchStartonTouchMove 方法中,多次调用了 setExactValue 更新 exactValue 状态。但在后续代码中,似乎没有对 exactValue 进行使用。这可能是一个冗余的状态更新。

如果确实不需要 exactValue,建议删除相关的状态声明和更新操作,以精简代码。

Also applies to: 286-286

Comment on lines +147 to +152
const scope = useMemo(() => {
if (max < min || max === min) {
console.log('max 的值需要大于 min的值')
}
return max - min
}, [max, min])
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

解决当 max === min 时的除零问题

max 等于 min 时,scope 将为零,可能导致后续计算中出现除零错误或 NaN 值。建议在这种情况下,直接返回或给出提示,避免计算错误。

可以考虑修改如下:

if (max < min || max === min) {
-   console.log('max 的值需要大于 min的值')
+   throw new Error('`max` 值必须大于 `min` 值')
}

Committable suggestion was skipped due to low confidence.

@oasis-cloud oasis-cloud merged commit 1b0fee4 into jdf2e:next Oct 12, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants