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(Transfer): Dropdown is hidden & showSizeChanger not work #41906

Merged
merged 4 commits into from Jun 28, 2023

Conversation

Yuiai01
Copy link
Contributor

@Yuiai01 Yuiai01 commented Apr 20, 2023

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Workflow
  • Other (about what?)

🔗 Related issue link

close #41790

💡 Background and solution

在当前的 Transfer 组件中,如果设置了 pagination={{ showSizeChanger: true, simple: false }} ,那么会出现切换分页数量的下拉按钮被隐藏的情况:
image
解决被隐藏问题并添加右边距(之前贴边)后,如下图:
image
根据 https://github.com/ant-design/ant-design/issues/41790#issuecomment-1507831602,发现 Transfer 组件内部的 Pagination 没有绑定 showSizeChanger 方法,因此添加上对应方法。

📝 Changelog

Language Changelog
🇺🇸 English fix Pagination Dropdown is hidden & showSizeChanger not work
🇨🇳 Chinese 修复分页下拉按钮被隐藏以及 showSizeChanger 方法无效

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

🚀 Summary

🤖 Generated by Copilot at ff72fad

Added pagination support for the Transfer component, allowing users to select and change the page size of the transfer list. Updated the test cases, the TransferList and ListBody components, and the style file to handle the pagination functionality.

🔍 Walkthrough

🤖 Generated by Copilot at ff72fad

  • Add pageSize and onPageSizeChange props to TransferList and ListBody components to enable changing the number of items per page in the transfer component (link, link, link, link, link, link)
  • Modify parsePagination function to accept an optional pageSize parameter and use it to slice the render items according to the current page number and page size (link, link, link)
  • Add onShowSizeChange prop to Pagination component to trigger the onPageSizeChange callback and reset the current page number when the user selects a different page size (link)
  • Add generateData function to index.test.tsx to create mock data items for testing the transfer component with pagination (link)
  • Add test case for changing the page size in the transfer component and verify the correct number of items per page (link)
  • Add antCls parameter to genTransferListStyle function in style/index.ts to access the ant design class name for the transfer list component (link)
  • Add minHeight property to the transfer list style to fix a flexbox bug that causes the list body to overflow the list container when the page size is larger than the number of items (link)
  • Add paddingInlineEnd property to the pagination options style to add some spacing between the page size changer and the pagination buttons (link)

@github-actions
Copy link
Contributor

github-actions bot commented Apr 20, 2023

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c3225c0) 100.00% compared to head (889f16e) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master    #41906   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          648       648           
  Lines        10965     10967    +2     
  Branches      2978      2978           
=========================================
+ Hits         10965     10967    +2     
Impacted Files Coverage Δ
components/transfer/list.tsx 100.00% <ø> (ø)
components/transfer/style/index.ts 100.00% <ø> (ø)
components/transfer/ListBody.tsx 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

}

const parsePagination = (pagination?: PaginationType) => {
const parsePagination = (pagination?: PaginationType, pageSize?: number) => {
Copy link
Member

Choose a reason for hiding this comment

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

pageSize 应该包含在 pagination 里?

Copy link
Contributor Author

@Yuiai01 Yuiai01 Apr 21, 2023

Choose a reason for hiding this comment

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

放在里面的话会出现切换 pageSize 后页面数据不刷新,所以放到外面了

Copy link
Member

Choose a reason for hiding this comment

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

这很奇怪,不应该单独维护一个 pageSize,可以看看什么原因导致

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这很奇怪,不应该单独维护一个 pageSize,可以看看什么原因导致

ListBody 内部通过 ref 将过滤好的数据传递给外层,如果设置 showSizeChanger 后在 ListBody 内部维护 pageSize 那么就会出现我切换完 pageSize 后页面数据不会变化,因为没有触发外层组件的刷新。因此我将 pageSize 提取出来单独维护了

Copy link
Member

@MadCcc MadCcc Apr 26, 2023

Choose a reason for hiding this comment

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

那应该还是合进去,这样的话参数冗余了。使用的时候可以这样

parsePagination({ ...pagination, pageSize})

@afc163
Copy link
Member

afc163 commented Apr 26, 2023

any progress?

@Yuiai01 Yuiai01 force-pushed the fix-transfer branch 2 times, most recently from 8c91736 to 0ccaee7 Compare April 26, 2023 08:39
@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Apr 26, 2023

any progress?

好了~

const maxPageCount = Math.ceil(filteredRenderItems.length / mergedPagination.pageSize!);
setCurrent(Math.min(current, maxPageCount));
}
}, [filteredRenderItems, pagination]);
}, [filteredRenderItems, pagination, pageSize]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, [filteredRenderItems, pagination, pageSize]);
}, [filteredRenderItems, convertPagination, pageSize]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

修改掉了


const [current, setCurrent] = React.useState<number>(1);
const [pageSize, setPageSize] = React.useState<number>(convertPagination?.pageSize ?? 10);
Copy link
Member

@MadCcc MadCcc Apr 26, 2023

Choose a reason for hiding this comment

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

如果 pagination 里传进来的 pageSize 改变了是否应该响应?如果是的话应该用 useMergeState 代理受控

Copy link
Contributor Author

Choose a reason for hiding this comment

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

嗯嗯,但是这样做的话,就始终使用传入的 pagesize,不可更改了,酱紫是正常的么

const memoizedItems = React.useMemo<RenderedItem<RecordType>[]>(() => {
const mergedPagination = parsePagination(pagination);
const displayItems = mergedPagination
const mergedPagination = parsePagination({ ...convertPagination, pageSize });
Copy link
Member

Choose a reason for hiding this comment

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

感觉这一行写了很多遍,可以单独写一个 useMemo 吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好,用 useMemo 封装了一下

Comment on lines -20 to -22
if (!pagination) {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

这里判空还是得有?

onScroll,
onItemSelect,
onItemRemove,
} = props;
const convertPagination = typeof pagination === 'object' ? pagination : {};
Copy link
Member

Choose a reason for hiding this comment

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

感觉这里不等价,pagination 可以是 false 的话就和 true 一样处理了。
并且 null 也是 object

Copy link
Member

Choose a reason for hiding this comment

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

我是觉得应该一并收到 mergedPagination 这个 memo 里处理好点?

@MadCcc
Copy link
Member

MadCcc commented Apr 30, 2023

受控还是要用 useMergedState,props 里的作为 value,最终在下面使用的都是 pageSize 这个 state 而不是 mergedPagination 里的 pageSize。

@YMNNs
Copy link

YMNNs commented Jun 24, 2023

@MadCcc 可以考虑合并一下这个PR吗?

@afc163
Copy link
Member

afc163 commented Jun 24, 2023

@YMNNs ci 挂了

@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Jun 26, 2023

@MadCcc 可以考虑合并一下这个PR吗?

我这边跟进一下~

@MadCcc
Copy link
Member

MadCcc commented Jun 28, 2023

@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Jun 28, 2023

argos 截图好像变了 https://app.argos-ci.com/ant-design/ant-design/builds/9547/50739793

好,我康康

@MadCcc MadCcc merged commit 91be823 into ant-design:master Jun 28, 2023
51 of 52 checks passed
@afc163
Copy link
Member

afc163 commented Jun 28, 2023

图片

这个样式这么乱,感觉不太对。

@Yuiai01
Copy link
Contributor Author

Yuiai01 commented Jun 28, 2023

图片 这个样式这么乱,感觉不太对。

这个样式是自定义出来的,可能自定义的参数与普通的组件样式差异过大所以看起来比较乱。
image

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.

Transfer 穿梭框分页 SizeChanger 无效且缺少左右 padding 样式
4 participants