-
Notifications
You must be signed in to change notification settings - Fork 291
refactor(countup): 优化数字滚动动画实现逻辑 #3398
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
base: feat_v3.x
Are you sure you want to change the base?
Conversation
走查总结本PR将计数动画组件中基于DOM的转换计算替换为数组驱动的方法,移除了createSelectorQuery和DOM遍历逻辑,并扩展了useCallback依赖项以确保duration变化时转换重新计算。 变更内容
预估代码审查工作量🎯 3 (中等) | ⏱️ ~20 分钟
可能相关的PR
建议审查人
诗歌
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this 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
🧹 Nitpick comments (2)
src/packages/animatingnumbers/countup.taro.tsx (2)
56-74: 基于数字的滚动计算逻辑整体 OK,但位移比例建议抽象为可推导常量这段重写后的逻辑直接基于
numberArr的字符值计算 transform,避免了 DOM 查询和布局依赖,能稳定保证显示值与value保持一致,方向是很好的。有两个小建议可考虑:
位移比例的魔法数 5%
当前写死为numValue * 5%,这个 5% 显然与[...numbers, ...numbers]的长度(当前是 20 个元素)耦合。如果未来调整数字重复次数或样式高度,这里容易被遗忘。可以考虑用长度推导,避免魔法数,例如类似:const itemHeightPercent = 100 / (numbers.length * 2) // 当前为 5 // ... transform: `translate(0, -${numValue * itemHeightPercent}%)`,这样即使后面调整数字数组长度,也不需要手动同步 5%。
构造数组方式的小优化(非必须)
现在是const newTransformArr: CSSProperties[] = []然后forEach逐个赋值。由于idx与numberArr一一对应,也可以用map直接返回新数组,更直观一些(功能等价,不是必须改):const newTransformArr = numberArr.map((item) => { const numValue = Number(item) return Number.isNaN(numValue) ? undefined : { transitionDuration: `${duration}s`, transform: `translate(0, -${numValue * itemHeightPercent}%)`, } })整体上这块逻辑功能是正确的,这些只是降低未来维护成本的优化点。
76-92: useEffect 中isLoaded标志位的实际效果和预期可能不一致,建议确认并视情况简化逻辑当前 effect 依赖为
[numberArr, delay, setNumberTransform],并且在清理函数中有:return () => { clearTimeout(timerRef.current) isLoaded.current = false }这意味着:
- 每次
numberArr(或delay/setNumberTransform)变化时,旧 effect 的 cleanup 会先执行,把isLoaded.current重置为false。- 随后新 effect 运行时,
!isLoaded.current恒为true,因此始终走setTimeout分支;else分支里的setNumberTransform()实际上几乎不会被命中。如果你的设计是「每次 value / numberArr 变化都需要经过
delay再开始滚动」,那么现在的行为是正确的,但可以考虑直接去掉isLoaded和else分支,只保留统一的定时逻辑,简化状态:useEffect(() => { if (numberArr.length) { // @ts-ignore timerRef.current = setTimeout(() => { setNumberTransform() }, delay) } return () => { clearTimeout(timerRef.current) } }, [numberArr, delay, setNumberTransform])如果原来的意图是「首屏延迟,后续变更立即滚动」,则需要重新设计
isLoaded的使用位置(例如不在 cleanup 中重置,或拆成两个 effect)。另外,这里已经在 cleanup 中统一
clearTimeout,符合我们之前关于定时器必须清理以避免内存泄漏的经验,这是好的实践。Based on learnings, …
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/packages/animatingnumbers/countup.taro.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oasis-cloud
Repo: jdf2e/nutui-react PR: 2700
File: src/packages/animatingnumbers/animatingnumbers.harmony.css:25-32
Timestamp: 2024-11-06T05:56:06.800Z
Learning: 在优化 NutUI React 动画性能时,添加 `will-change` 属性可能会对布局产生影响,需要谨慎使用。
📚 Learning: 2025-05-02T01:45:09.576Z
Learnt from: irisSong
Repo: jdf2e/nutui-react PR: 3209
File: src/packages/searchbar/searchbar.taro.tsx:111-124
Timestamp: 2025-05-02T01:45:09.576Z
Learning: 在 React/Taro 组件中使用 setTimeout 或 setInterval 时,应当在组件卸载时通过 useEffect 的清理函数清除定时器,以防止内存泄漏。可以使用 useState 存储定时器 ID,并在 useEffect 的返回函数中清除。
Applied to files:
src/packages/animatingnumbers/countup.taro.tsx
🤔 这个变动的性质是?
🔗 相关 Issue
#3397
💡 需求背景和解决方案
AnimatingNumbers 数字动画组件显示值与 value 不一致问题
☑️ 请求合并前的自查清单
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.