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.

Sign up for GitHub

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: avoid table jitter casued by changes in the first row #301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: avoid table jitter casued by changes in the first row #301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/List.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type { ExtraRenderInfo, GetKey, RenderFunc, SharedConfig } from './interf
import type { ScrollBarDirectionType, ScrollBarRef } from './ScrollBar';
import ScrollBar from './ScrollBar';
import { getSpinSize } from './utils/scrollbarUtil';
import { debounce } from './utils/debounce';

const EMPTY_DATA = [];

Expand Down Expand Up @@ -271,10 +272,15 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) {
rangeRef.current.start = start;
rangeRef.current.end = end;

const isScrollingRef = useRef(false);
// When scroll up, first visible item get real height may not same as `itemHeight`,
// Which will make scroll jump.
// Let's sync scroll top to avoid jump
React.useLayoutEffect(() => {
// When the `scrollHeight` change is not caused by scrolling,
// end the function execution avoiding table jitter caused by changes in the first row
if (!isScrollingRef.current) return;

const changedRecord = heights.getRecord();
if (changedRecord.size === 1) {
const recordKey = Array.from(changedRecord)[0];
Expand Down Expand Up @@ -377,6 +383,9 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) {
}
}

const toggleScrollStatus = React.useCallback(debounce(() => {
isScrollingRef.current = false;
}, 100), []);
// When data size reduce. It may trigger native scroll event back to fit scroll position
function onFallbackScroll(e: React.UIEvent<HTMLDivElement>) {
const { scrollTop: newScrollTop } = e.currentTarget;
Expand All @@ -387,6 +396,10 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) {
// Trigger origin onScroll
onScroll?.(e);
triggerScroll();
// Set the scroll status to `true`
isScrollingRef.current = true;
// Set the scroll status to `false` after scrolling ends
toggleScrollStatus();
Comment on lines +399 to +402
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议添加滚动状态重置的清理逻辑

在组件卸载时需要确保滚动状态被正确重置。

+useEffect(() => {
+  return () => {
+    isScrollingRef.current = false;
+    toggleScrollStatus.cancel();
+  };
+}, []);

Committable suggestion skipped: line range outside the PR's diff.

}

const keepInHorizontalRange = (nextOffsetLeft: number) => {
Expand Down
10 changes: 10 additions & 0 deletions src/utils/debounce.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export function debounce(fn: (...args) => void, delay: number = 300) {
let timer;
return function (...args) {
if (timer) clearTimeout(timer);
timer = setTimeout(() => {
fn.apply(this, args);
timer = null;
}, delay);
}
}
Comment on lines +1 to +10
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

需要处理定时器清理以防止内存泄漏

当前的防抖函数实现可能会导致内存泄漏，因为定时器没有在适当的时候被清理。

建议按照以下方式重构代码：

 export function debounce(fn: (...args) => void, delay: number = 300) {
   let timer;
-  return function (...args) {
+  const debounced = function (...args) {
     if (timer) clearTimeout(timer);
     timer = setTimeout(() => {
       fn.apply(this, args);
       timer = null;
     }, delay);
+  };
+  
+  debounced.cancel = () => {
+    if (timer) {
+      clearTimeout(timer);
+      timer = null;
+    }
   };
+  
+  return debounced;
 }
📝 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
export function debounce(fn: (...args) => void, delay: number = 300) {
let timer;
return function (...args) {
if (timer) clearTimeout(timer);
timer = setTimeout(() => {
fn.apply(this, args);
timer = null;
}, delay);
}
}
export function debounce(fn: (...args) => void, delay: number = 300) {
let timer;
const debounced = function (...args) {
if (timer) clearTimeout(timer);
timer = setTimeout(() => {
fn.apply(this, args);
timer = null;
}, delay);
};
debounced.cancel = () => {
if (timer) {
clearTimeout(timer);
timer = null;
}
};
return debounced;
}

Loading