-
Notifications
You must be signed in to change notification settings - Fork 273
feat(infiniteloading): add refreshDistance props and refactor pull-to-refresh trigger mechanism in taro #3112
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
Walkthrough此次更改主要针对 Changes
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat_v3.x #3112 +/- ##
=============================================
- Coverage 86.60% 86.60% -0.01%
=============================================
Files 289 289
Lines 18742 18739 -3
Branches 2821 2821
=============================================
- Hits 16232 16229 -3
Misses 2505 2505
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 (1)
src/packages/infiniteloading/infiniteloading.taro.tsx (1)
54-63
: 新增的 getRectTaro 函数需要改进错误处理和类型定义将 Taro 的回调式 API 包装成 Promise 是一个很好的做法,提高了代码的可读性和可维护性。但是当前实现存在两个问题:
- 缺少错误处理机制,如果查询失败没有 reject 处理
- 返回类型为
Promise<any>
,类型安全性不够建议改进错误处理和类型定义:
- const getRectTaro = async (selector: string): Promise<any> => { + interface RectResult { + width?: number + height?: number + top?: number + bottom?: number + left?: number + right?: number + } + + const getRectTaro = async (selector: string): Promise<RectResult> => { return new Promise((resolve) => { createSelectorQuery() .select(selector) .boundingClientRect() .exec((res: any) => { - resolve(res[0]) + if (res && res[0]) { + resolve(res[0]) + } else { + resolve({}) + } }) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/packages/infiniteloading/infiniteloading.taro.tsx
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (6)
src/packages/infiniteloading/infiniteloading.taro.tsx (6)
9-9
: 好的改进,增加了更灵活的属性合并方式引入
mergeProps
工具函数是一个很好的做法,相比简单的对象展开运算符,它可以提供更强大的属性合并功能,有助于确保属性的正确继承和覆盖。
15-15
: 将触发阈值从 40 调整为 200 的合理性需要确认将默认的
threshold
值从 40 增加到 200 是一个较大的变化,这会影响下拉刷新的触发灵敏度。这个改动可能基于用户体验测试或特定设备适配的需求,但值得确认这个数值是否经过充分测试。这个阈值变化是否在不同设备上进行了测试验证?较高的阈值可能在某些小屏幕设备上需要用户滑动更长距离才能触发加载。
41-41
: 使用 mergeProps 替代展开运算符是良好的实践使用
mergeProps
函数代替简单的对象展开运算符是一个更加稳健的属性合并方式。这样的改变保持了代码库的一致性,并可能更好地处理特殊的属性合并情况。
66-70
: 异步处理元素高度的方法得到了改进使用
async/await
和新的getRectTaro
函数获取元素高度是一个很好的改进,代码可读性更高。使用可选链操作符(?.
)和空值合并操作符(??
)也是处理可能不存在的值的良好实践。
72-72
: 依赖数组的更新使副作用逻辑更加清晰将依赖数组从可能包含
threshold
改为只依赖[hasMore, isInfiniting]
是合理的,因为计算过程似乎不依赖于threshold
值的变化。这种更改使得组件的副作用逻辑更加清晰和可预测。
174-174
: 明确指定 lowerThreshold 属性增强了代码可读性明确为 ScrollView 组件设置
lowerThreshold
属性而不是依赖属性展开是一个很好的做法,这使得代码更加清晰,明确了threshold
属性的用途。
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 (1)
src/packages/infiniteloading/infiniteloading.tsx (1)
12-47
: 建议添加一个验证函数为了确保 refreshDistance 的值始终为正数且合理,建议添加一个简单的验证函数。
const { children, type, hasMore, threshold, target, capture, pullRefresh, pullingText, loadingText, loadMoreText, refreshDistance, className, onRefresh, onLoadMore, onScroll, ...restProps } = mergeProps(defaultProps, props) + // 确保 refreshDistance 为正数且有合理值 + const validRefreshDistance = Math.max(1, refreshDistance || 100)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/packages/infiniteloading/doc.en-US.md
(1 hunks)src/packages/infiniteloading/doc.md
(1 hunks)src/packages/infiniteloading/doc.taro.md
(1 hunks)src/packages/infiniteloading/doc.zh-TW.md
(1 hunks)src/packages/infiniteloading/infiniteloading.taro.tsx
(7 hunks)src/packages/infiniteloading/infiniteloading.tsx
(5 hunks)src/sites/sites-react/doc/docs/react/migrate-from-v2.md
(1 hunks)src/sites/sites-react/doc/docs/taro/migrate-from-v2.md
(1 hunks)src/types/spec/infiniteloading/base.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (21)
src/types/spec/infiniteloading/base.ts (1)
11-11
: 新增的 refreshDistance 属性很合理向 BaseInfiniteLoading 接口添加 refreshDistance 属性清晰地表达了下拉刷新触发的距离阈值。这个变更与 PR 的目标一致,有助于改进组件的下拉刷新机制。
src/packages/infiniteloading/doc.en-US.md (1)
68-68
: 文档更新与接口变更一致新增的 refreshDistance 属性文档说明清晰,类型和默认值与接口定义匹配。文档描述"Pull refresh trigger distance"准确地表达了该属性的用途。
src/packages/infiniteloading/doc.md (1)
68-68
: 中文文档更新恰当新增的 refreshDistance 属性文档说明清晰,类型和默认值(100)与接口定义匹配。"下拉刷新触发距离"的描述准确表达了该属性的功能。
src/packages/infiniteloading/doc.zh-TW.md (1)
68-68
: 繁体中文文档更新完善新增的 refreshDistance 属性文档说明清晰,类型和默认值(100)与其他语言版本保持一致。"下拉刷新觸發距離"的描述准确传达了该属性的用途。
src/packages/infiniteloading/doc.taro.md (1)
59-59
: 很好!清晰地文档化了新增的refreshDistance
属性属性描述和默认值与实现匹配,使开发者能够根据需要自定义下拉刷新触发距离。
src/sites/sites-react/doc/docs/taro/migrate-from-v2.md (1)
222-222
: 文档更新完善在升级指南中添加了新增的
refreshDistance
属性,确保用户在迁移过程中了解这个变更。src/sites/sites-react/doc/docs/react/migrate-from-v2.md (1)
219-219
: 文档保持一致性React 版本的迁移文档中也同步添加了
refreshDistance
属性,保持了文档的一致性。src/packages/infiniteloading/infiniteloading.tsx (5)
6-6
: 使用 mergeProps 合并属性引入 mergeProps 工具函数来合并默认属性和用户传入的属性,这是一个良好的实践。
20-20
: 添加 refreshDistance 默认值设置默认值为 100,与文档描述一致。
40-46
: 正确提取 refreshDistance 属性并使用 mergeProps解构出 refreshDistance 属性并使用 mergeProps 合并默认属性和用户传入的属性。
129-131
: 优化下拉刷新触发机制将原来的使用 refreshMaxH.current 的判断逻辑改为使用可配置的 refreshDistance,提高了组件的灵活性。
143-143
: 统一使用 refreshDistance 判断在 touchEnd 中也统一使用 refreshDistance 进行判断,保持逻辑一致性。
src/packages/infiniteloading/infiniteloading.taro.tsx (9)
9-10
: 提取公共方法改善代码组织导入并使用工具函数
mergeProps
和getRectByTaro
是一个很好的改进,这解决了之前评论中提到的"这个应该有个公共方法"的问题。提取这些功能到公共方法可以提高代码的可维护性和复用性。
16-19
: 阈值调整和新属性添加将
threshold
从 40 调整为 200 可能会影响滚动触发加载的时机,这是一个潜在的破坏性变更。新增的refreshDistance
属性为下拉刷新提供了更好的可配置性,但请确保在文档中明确说明这些变更。请确认是否已更新相关文档,以便用户了解这些配置参数的变化。
42-44
: 使用 mergeProps 替代展开运算符使用
mergeProps
函数替代展开运算符是一个很好的做法,它可以更好地处理默认属性和传入属性的合并,特别是在处理复杂对象时。
54-54
: 添加 ref 以获取元素尺寸为 refreshTips 添加 ref 是一个好的做法,这样可以准确获取元素的尺寸,而不是依赖于 class 或 id 选择器,这也解决了之前评论中提到的"采用 id 的方式处理,class 会出现选不对的情况"的问题。
58-65
: useEffect 依赖项和异步逻辑改进在 useEffect 中使用异步函数获取元素尺寸是合适的,并且清除定时器的逻辑也很完善。但是,从依赖数组中移除
threshold
是否恰当需要确认,因为这可能会影响效果的执行时机。我注意到
refreshMaxH.current
的计算逻辑已更新,现在基于元素的实际高度,这提高了组件的适应性。请确认从依赖数组中移除
threshold
不会导致问题,因为如果threshold
变化可能需要重新计算相关值。
128-131
: 下拉刷新触发机制优化使用
refreshDistance
属性来确定下拉刷新的触发距离,而不是依赖于动态计算的refreshMaxH.current
,使得组件行为更加可预测和可配置。这是一个很好的改进,使组件更加灵活。
140-140
: 更新触发刷新的条件判断将
touchEnd
函数中的条件判断更新为使用refreshDistance
,与touchMove
中的逻辑保持一致,这提高了代码的一致性和可维护性。
167-167
: 明确设置 lowerThreshold 属性显式地将
threshold
值传递给 ScrollView 的lowerThreshold
属性,使得组件的行为更加明确和可预测。
176-176
: 为刷新提示添加 ref为刷新提示元素添加 ref 是一个很好的做法,这使得可以准确获取元素的尺寸,而不是依赖于选择器。
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/sites/sites-react/doc/docs/react/migrate-from-v2.en-US.md (1)
217-217
: 文档中新增refreshDistance
属性说明清晰
新增的refreshDistance
属性描述已明确指出其功能——用于触发下拉刷新。建议在描述中补充默认值信息(默认值为 100),以便开发者更好地理解组件的默认行为。src/sites/sites-react/doc/docs/taro/migrate-from-v2.en-US.md (1)
217-218
: 完善新增加的refreshDistance
属性说明建议在文档中补充
refreshDistance
属性的默认值(100)以及详细描述该属性在下拉刷新触发机制中的作用,以便用户能够清晰地理解此新增属性与原有功能(如threshold
)的区别和用法。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/packages/datepickerview/__test__/__snapshots__/datepickerview.spec.tsx.snap
is excluded by!**/*.snap
src/packages/infiniteloading/__tests__/__snapshots__/infiniteloading.spec.tsx.snap
is excluded by!**/*.snap
📒 Files selected for processing (2)
src/sites/sites-react/doc/docs/react/migrate-from-v2.en-US.md
(2 hunks)src/sites/sites-react/doc/docs/taro/migrate-from-v2.en-US.md
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
src/sites/sites-react/doc/docs/react/migrate-from-v2.en-US.md (1)
1-1
: 标题格式更新合理
已移除标题 "Upgrade from v2 to v3" 末尾的句点,使标题更简洁且符合文档风格。
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/infiniteloading/infiniteloading.taro.tsx (2)
56-60
: 异步处理和依赖项调整在
setTimeout
回调中使用了async
关键字,但getScrollHeight
函数并不返回 Promise,这个async
是不必要的。同时,从依赖数组中移除了threshold
,这意味着threshold
变化时不会重新执行该效果,确保这是有意为之的。- const timer = setTimeout(async () => { + const timer = setTimeout(() => { getScrollHeight() }, 200)
62-70
: 建议使用 async/await 或回调一致性处理异步操作
getScrollHeight
函数使用了回调风格的异步操作,但在某些地方(如第56行)被视为返回 Promise 的函数。建议统一异步处理风格,可以将其重构为返回 Promise 的函数。- const getScrollHeight = () => { + const getScrollHeight = (): Promise<void> => { + return new Promise((resolve) => { const parentElement = getParentElement('scroller') parentElement .boundingClientRect((rect: any) => { scrollHeight.current = rect?.height ?? 0 + resolve() }) .exec() + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/packages/infiniteloading/infiniteloading.taro.tsx
(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (6)
src/packages/infiniteloading/infiniteloading.taro.tsx (6)
9-9
: 使用 mergeProps 替代简单的对象扩展是一个很好的改进通过引入和使用
mergeProps
函数来合并属性,可以更健壮地处理组件属性的合并过程,避免一些常见的对象合并问题。这是一个良好的实践。Also applies to: 43-43
15-15
: 阈值调整和添加新属性需要验证其影响将
threshold
默认值从 40 增加到 200 是一个显著的变化,可能会影响滚动触发行为。新添加的refreshDistance
属性(默认值为 100)用于控制下拉刷新的触发距离。请确保这些值已经过充分测试,并且对用户体验有积极影响。您可以通过以下方式验证这些变更:
- 在不同尺寸的设备上测试组件行为
- 验证这些数值是否符合人体工程学(触发距离太短体验不佳,太长则操作困难)
- 确认这些默认值在各种使用场景下都合理
Also applies to: 18-18
111-118
: 触摸开始事件处理优化触摸开始事件的处理逻辑得到了改进,添加了距离初始化和滚动距离重置,使状态更加明确。
120-136
: 触摸移动事件处理逻辑简化触摸移动事件处理已被大幅简化,逻辑更加清晰:
- 计算触摸移动距离
- 确保不超过
refreshDistance
- 更新状态
新的实现更易于理解和维护。
138-150
: 触摸结束事件处理逻辑优化触摸结束事件处理逻辑更加简洁明了,基于
refreshDistance
决定是重置滚动还是触发刷新操作。代码结构清晰,逻辑易于理解。
169-169
: 显式传递 lowerThreshold 属性向 ScrollView 组件显式传递
lowerThreshold
属性是一个好的做法,使组件配置更明确,避免依赖默认行为。
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: 1
♻️ Duplicate comments (1)
src/packages/infiniteloading/infiniteloading.taro.tsx (1)
17-17
: 考虑属性命名一致性新增的
refreshDistance
属性用于控制下拉刷新的触发距离,功能清晰。考虑使用
headHeight
作为属性名,以与 pulltorefresh 组件的属性保持一致,提高整个组件库的一致性。
🧹 Nitpick comments (1)
src/packages/infiniteloading/infiniteloading.taro.tsx (1)
14-14
: 注意显著的阈值变更将默认的
threshold
值从 40 变更为 200 是一个显著的变化,这会影响无限加载的触发时机。此变更可能导致无限加载更早地触发,请确保这符合预期行为。建议添加注释解释为什么需要将阈值增加5倍,以便其他开发者理解这一变化的原因。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/packages/infiniteloading/infiniteloading.taro.tsx
(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/packages/infiniteloading/infiniteloading.taro.tsx (1)
src/utils/merge-props.ts (1)
mergeProps
(4-16)
🪛 GitHub Check: build
src/packages/infiniteloading/infiniteloading.taro.tsx
[failure] 42-42:
Cannot find name 'mergeProps'.
🪛 GitHub Actions: CI
src/packages/infiniteloading/infiniteloading.taro.tsx
[error] 42-42: TypeScript error TS2304: Cannot find name 'mergeProps'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (5)
src/packages/infiniteloading/infiniteloading.taro.tsx (5)
54-59
: 移除了threshold依赖项从
useEffect
的依赖数组中移除了threshold
,并简化了定时器代码。这是一个合理的改进,因为
threshold
不会影响滚动高度的计算。
110-117
: 改进下拉刷新的触发机制通过移除
scrollTop.current === 0
检查,简化了触发下拉刷新的条件。代码还新增了重置
distance.current
和setTopDisScoll(0)
的逻辑,这有助于确保下拉刷新从一个已知状态开始,提高了可靠性。
119-135
: 重构下拉距离计算逻辑触摸移动事件处理函数得到了显著重构,新实现使用了更清晰明确的方式计算和限制下拉距离。
新代码使用
Math.max
和Math.min
函数确保下拉距离在 0 和refreshDistance
之间,使逻辑更加健壮和直观。建议考虑添加注释解释计算逻辑,以便其他开发者更容易理解。
137-149
: 优化触摸结束处理函数触摸结束处理逻辑得到了重构,现在明确使用
refreshDistance
来决定是否触发刷新操作。新代码增加了提前返回的逻辑,避免不必要的处理,并确保在刷新完成后调用
refreshDone()
,这些改变使组件的行为更加可预测。
168-168
: 添加lowerThreshold属性为 ScrollView 组件添加了
lowerThreshold
属性,并设置为threshold
值。这确保了 ScrollView 组件能够正确使用传入的阈值,使组件行为更加一致。
Summary by CodeRabbit
refreshDistance
属性,支持自定义下拉刷新触发距离。refreshDistance
属性说明,详述下拉刷新触发距离配置。