Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough本次更改调整了滚动到目标元素的逻辑。现在，只有在 Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/hooks/useTarget.tsOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs+fabric@2.14.1/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
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. 🪧 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 (
|
|
😭 Deploy PR Preview f859b69 failed. Build logs
🤖 By surge-preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #78 +/- ##
=======================================
Coverage 99.04% 99.04%
=======================================
Files 9 9
Lines 209 209
Branches 95 95
=======================================
Hits 207 207
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/hooks/useTarget.ts (5)
44-79: 建议：考虑提取滚动相关的逻辑到单独的函数中
updatePos函数处理了几个不同的职责：确定是否需要滚动、获取目标元素位置、处理内联模式的位置调整等。为了提高代码可维护性，建议将滚动逻辑提取到一个单独的函数中。const updatePos = useEvent(() => { if (targetElement) { // 提取滚动逻辑到单独函数 + const scrollIfNeeded = () => { + if (!inlineMode && !isInViewPort(targetElement) && open) { + targetElement.scrollIntoView(scrollIntoViewOptions); + } + }; + + scrollIfNeeded(); - if (!inlineMode && !isInViewPort(targetElement) && open) { - targetElement.scrollIntoView(scrollIntoViewOptions); - } const { left, top, width, height } = targetElement.getBoundingClientRect(); // ... 剩余代码保持不变
84-95: 建议：优化事件监听器的性能目前
scroll和resize事件没有节流(throttle)处理，在快速滚动或调整窗口大小时可能导致性能问题。建议添加节流来限制更新频率。+ import throttle from '@rc-component/util/lib/hooks/useThrotte'; // 或者创建一个简单的节流函数 + const throttle = (fn, delay) => { + let lastCall = 0; + return function(...args) { + const now = new Date().getTime(); + if (now - lastCall < delay) { + return; + } + lastCall = now; + return fn(...args); + }; + }; useLayoutEffect(() => { + const throttledUpdatePos = throttle(updatePos, 100); // 100ms的节流 updatePos(); // 更新时使用节流函数 - window.addEventListener('resize', updatePos); - window.addEventListener('scroll', updatePos); + window.addEventListener('resize', throttledUpdatePos); + window.addEventListener('scroll', throttledUpdatePos); return () => { - window.removeEventListener('resize', updatePos); - window.removeEventListener('scroll', updatePos); + window.removeEventListener('resize', throttledUpdatePos); + window.removeEventListener('scroll', throttledUpdatePos); }; }, [targetElement, open, updatePos]);
87-94: 建议：使用useEventListener或类似工具来管理事件监听对于事件监听的添加和移除，可以考虑使用
useEventListener这样的钩子来简化代码并减少重复。
38-42: 注意：无依赖项的useLayoutEffect可能引起不必要的重渲染这个
useLayoutEffect钩子没有依赖数组，意味着它会在每次组件渲染后执行。建议添加适当的依赖项，例如[target]，只在target改变时更新targetElement。- useLayoutEffect(() => { + useLayoutEffect(() => { const nextElement = typeof target === 'function' ? (target as any)() : target; setTargetElement(nextElement || null); - }); + }, [target]);
69-74: 建议：使用对象比较而非JSON.stringify使用
JSON.stringify比较对象结构是低效的，并且可能在处理包含特殊值（如循环引用）的对象时失败。建议使用深度比较函数或者直接比较各个属性。setPosInfo(origin => { - if (JSON.stringify(origin) !== JSON.stringify(nextPosInfo)) { + if (!origin || + origin.left !== nextPosInfo.left || + origin.top !== nextPosInfo.top || + origin.width !== nextPosInfo.width || + origin.height !== nextPosInfo.height) { return nextPosInfo; } return origin; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useTarget.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/hooks/useTarget.ts (1)
src/util.ts (1)
isInViewPort(3-10)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview
| if (targetElement) { | ||
| // Exist target element. We should scroll and get target position | ||
| if (!isInViewPort(targetElement) && open) { | ||
| if (!inlineMode && !isInViewPort(targetElement) && open) { |
There was a problem hiding this comment.
修复了内联模式下的无限滚动循环问题，很好的改动！
此更改通过添加!inlineMode条件解决了一个关键问题。在内联模式下，元素的位置计算与常规模式不同（相对于父元素计算），之前的代码在内联模式下也会尝试滚动元素到视图中，这可能导致：
- 滚动触发 ->
- 触发scroll事件 ->
- 再次执行updatePos ->
- 元素仍然"不在视口中"（因为inline模式计算方式不同）->
- 再次触发滚动
从而形成死循环。此修复正确地阻止了内联模式下的自动滚动行为。
1 participant
修复 inline 模式下的滚动锁死问题，另外 jsdom 模拟不出来这个效果。要到 antd 里再测一遍。
Summary by CodeRabbit