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: adjust inVirtual logic(#227) #262

Merged
merged 6 commits into from
Apr 5, 2024
Merged

fix: adjust inVirtual logic(#227) #262

merged 6 commits into from
Apr 5, 2024

Conversation

NameWjp
Copy link
Contributor

@NameWjp NameWjp commented Mar 29, 2024

复现步骤:

  1. 修改 examples 中 height 例子,将 data 数据调整为 10 条,目前能够滚动,但是没有滚动条
  2. 调整逻辑后滚动条正常显示

close #243
close ant-design/ant-design#45935 again

Copy link

vercel bot commented Mar 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
virtual-list ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 5, 2024 4:59pm

@beautiful-boyyy
Copy link

是不是你antd版本太低了,最新版本没这问题了

@NameWjp
Copy link
Contributor Author

NameWjp commented Apr 1, 2024

@beautiful-boyyy 不是,你按照我的步骤是可以复现的,只是这个问题出现的概率较小(实际上就是你传入的:最小高度 * 数量 < 容器高度,但是 item 每一项的实际高度相加后 > 容器高度时,就会出现这个问题),最新版本的 antd 的虚拟滚动都有这个问题

@beautiful-boyyy
Copy link

@beautiful-boyyy 不是,你按照我的步骤是可以复现的,只是这个问题出现的概率较小(实际上就是你传入的:最小高度 * 数量 < 容器高度,但是 item 每一项的实际高度相加后 > 容器高度时,就会出现这个问题),最新版本的 antd 的虚拟滚动都有这个问题

提供一个codesandbox demo吧

@NameWjp
Copy link
Contributor Author

NameWjp commented Apr 1, 2024

@beautiful-boyyy 不是,你按照我的步骤是可以复现的,只是这个问题出现的概率较小(实际上就是你传入的:最小高度 * 数量 < 容器高度,但是 item 每一项的实际高度相加后 > 容器高度时,就会出现这个问题),最新版本的 antd 的虚拟滚动都有这个问题

提供一个codesandbox demo吧

codesandbox

@beautiful-boyyy
Copy link

@beautiful-boyyy 不是,你按照我的步骤是可以复现的,只是这个问题出现的概率较小(实际上就是你传入的:最小高度 * 数量 < 容器高度,但是 item 每一项的实际高度相加后 > 容器高度时,就会出现这个问题),最新版本的 antd 的虚拟滚动都有这个问题

提供一个codesandbox demo吧

codesandbox

貌似没权限

@NameWjp
Copy link
Contributor Author

NameWjp commented Apr 1, 2024

@beautiful-boyyy 不是,你按照我的步骤是可以复现的,只是这个问题出现的概率较小(实际上就是你传入的:最小高度 * 数量 < 容器高度,但是 item 每一项的实际高度相加后 > 容器高度时,就会出现这个问题),最新版本的 antd 的虚拟滚动都有这个问题

提供一个codesandbox demo吧

codesandbox

貌似没权限

忘记开了,再试下

@beautiful-boyyy
Copy link

@beautiful-boyyy 不是,你按照我的步骤是可以复现的,只是这个问题出现的概率较小(实际上就是你传入的:最小高度 * 数量 < 容器高度,但是 item 每一项的实际高度相加后 > 容器高度时,就会出现这个问题),最新版本的 antd 的虚拟滚动都有这个问题

提供一个codesandbox demo吧

codesandbox

你提供的demo没有用到antd List,antd List没有暴露 height,itemHeight接口,之前的bug也在前几个版本就修复了,virtual-list这里的逻辑确实有问题,单独使用的话确实会有bug,我之前也提了一个pr,但是测试没跑过,你可能需要提供正确的测试用例。

@NameWjp
Copy link
Contributor Author

NameWjp commented Apr 3, 2024

@zombieJ @yoyo837 这个 bug 可以看一下吗,需要咋调整下?

src/List.tsx Outdated
// ================================= MISC =================================
const useVirtual = !!(virtual !== false && height && itemHeight);
const inVirtual = useVirtual && data && (itemHeight * data.length > height || !!scrollWidth);
const containerHeight = React.useMemo(() => Object.values(heights.maps).reduce((total, curr) => total + curr, 0), [heights.id])
Copy link
Member

Choose a reason for hiding this comment

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

heights.maps 不需要 memo deps吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heights.maps 引用压根不会变,不过确实加上更好点

src/List.tsx Outdated Show resolved Hide resolved
@yoyo837
Copy link
Member

yoyo837 commented Apr 3, 2024

用例能覆盖到吗?

src/List.tsx Fixed Show fixed Hide fixed
@NameWjp
Copy link
Contributor Author

NameWjp commented Apr 4, 2024

用例能覆盖到吗?

测试已经加上了,本地跑的没啥问题

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.24%. Comparing base (e8e9aeb) to head (306dfcd).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #262   +/-   ##
=======================================
  Coverage   98.23%   98.24%           
=======================================
  Files          18       18           
  Lines         681      684    +3     
  Branches      160      163    +3     
=======================================
+ Hits          669      672    +3     
  Misses         12       12           

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

@@ -307,6 +308,8 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) {
const getVirtualScrollInfo = () => ({
x: isRTL ? -offsetLeft : offsetLeft,
y: offsetTop,
maxScrollWidth: !!scrollWidth ? scrollWidth - size.width : 0,
maxScrollHeight: scrollHeight > height ? maxScrollHeight : 0,
Copy link
Member

Choose a reason for hiding this comment

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

这个的计算我怎么有点没看懂

Copy link
Contributor Author

@NameWjp NameWjp Apr 5, 2024

Choose a reason for hiding this comment

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

maxScrollWidth:scrollWidth 可能不传,所以这里加了个判断。
maxScrollHeight:这里 scrollHeight 是取的 fillerInnerRef.current?.offsetHeight(感觉这里应该取容器的 scrollHeight),会出现 scrollHeight 小于容器 height 的情况,所以要做个判断

Copy link
Member

Choose a reason for hiding this comment

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

这俩属性和bugfix无关,我发PR去掉先。

Copy link
Member

Choose a reason for hiding this comment

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

@yoyo837 yoyo837 merged commit 34ec0e7 into react-component:master Apr 5, 2024
10 checks passed
@NameWjp
Copy link
Contributor Author

NameWjp commented Apr 18, 2024

@yoyo837 麻烦问下有发版计划吗,有个项目用的这个包,想修复这个问题

@yoyo837
Copy link
Member

yoyo837 commented Apr 18, 2024

已知会Owner.

@@ -307,6 +308,8 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) {
const getVirtualScrollInfo = () => ({
x: isRTL ? -offsetLeft : offsetLeft,
y: offsetTop,
maxScrollWidth: !!scrollWidth ? scrollWidth - size.width : 0,
Copy link
Member

Choose a reason for hiding this comment

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

尽量避免给测试用的打孔,这个去掉吧。测试换种方式测

Copy link
Member

Choose a reason for hiding this comment

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

@yoyo837
Copy link
Member

yoyo837 commented Apr 18, 2024

@yoyo837 麻烦问下有发版计划吗,有个项目用的这个包,想修复这个问题

https://github.com/react-component/virtual-list/releases/tag/v3.11.5

@NameWjp
Copy link
Contributor Author

NameWjp commented Apr 18, 2024

@yoyo837 麻烦问下有发版计划吗,有个项目用的这个包,想修复这个问题

https://github.com/react-component/virtual-list/releases/tag/v3.11.5

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

当下拉列表里面有9条或10条数据的时候,Select 组件的滚动条不会出现
4 participants