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

Pagination size selector props are grouped with the pageSizeChanger prop on rc-pagination #597

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

prakashks20
Copy link

Ant Design Pagination enhanced feature added in ant-design/ant-design#46757 depends on this PR changes

Copy link

vercel bot commented Jul 25, 2024

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

Name Status Preview Comments Updated (UTC)
pagination ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 5, 2024 3:43pm

@prakashmallow
Copy link

prakashmallow commented Jul 31, 2024

Waiting for the code review of this pull request. This pull request depends on fixing the Ant Design pagination issue

Comment on lines 17 to 22
showSizeChanger
pageSizeChanger={{
options: [10, 25, 50, 75, 100],
showSearch: false,
onChange: pageSizeOnChange,
}}
Copy link
Member

@afc163 afc163 Jul 31, 2024

Choose a reason for hiding this comment

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

Suggested change
showSizeChanger
pageSizeChanger={{
options: [10, 25, 50, 75, 100],
showSearch: false,
onChange: pageSizeOnChange,
}}
showSizeChanger={{
options: [10, 25, 50, 75, 100],
showSearch: false,
onChange: pageSizeOnChange,
}}

Instead of adding a new pageSizeChanger prop, we could reuse the showSizeChanger prop for accepting object value too.

Copy link
Author

Choose a reason for hiding this comment

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

@afc163 Will change and update this pull request as soon as possible.

Choose a reason for hiding this comment

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

@afc163 @MadCcc PR review changes updated. Could you please review these changes?

Choose a reason for hiding this comment

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

@MadCcc @afc163 The test case coverage failed issue has been fixed and updated on this PR. Could you please review this PR?

prakashmallow and others added 2 commits August 5, 2024 15:10
…Changer-option-for-pagination

Fixed for test cases coverage failed on the GitHub actions
src/interface.ts Outdated Show resolved Hide resolved
src/interface.ts Outdated Show resolved Hide resolved
src/interface.ts Outdated Show resolved Hide resolved
@prakashmallow
Copy link

@afc163 Code review changes have been addressed. Please review these changes.

@prakashks20
Copy link
Author

@afc163 @MadCcc @zombieJ Waiting for the changes in this pull request to fix the Antd pagination size selector issue. Could you please review it as soon as possible?

quickGo={shouldDisplayQuickJumper ? handleChange : null}
goButton={gotoButton}
onChange={typeof showSizeChanger === 'object' && showSizeChanger?.onChange}
Copy link
Member

Choose a reason for hiding this comment

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

changeSize and onChange is basicly same function, we should unify them to one

Copy link
Author

Choose a reason for hiding this comment

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

@afc163 Can I remove onChange from showSizeChanger?

Copy link
Member

Choose a reason for hiding this comment

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

LGTM, Pagination's onChange could be enough in most cases.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will remove it

@@ -588,9 +588,11 @@ const Pagination: React.FC<PaginationProps> = (props) => {
selectPrefixCls={selectPrefixCls}
changeSize={showSizeChanger ? changePageSize : null}
pageSize={pageSize}
pageSizeOptions={pageSizeOptions}
pageSizeOptions={(typeof showSizeChanger === 'object' && showSizeChanger?.options) || pageSizeOptions}
Copy link
Member

Choose a reason for hiding this comment

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

showSizeChanger should be assigned to a default object value if showSizeChanger={true}. Then we don't need typeof showSizeChanger here.

Copy link
Member

Choose a reason for hiding this comment

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

ping

Comment on lines 65 to +68
changeSize?.(Number(value));
if (onChange && typeof onChange === 'function') {
onChange?.(Number(value));
}
Copy link
Member

Choose a reason for hiding this comment

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

changeSize and onChange is basicly same function, we should unify them to one.

@@ -128,7 +135,7 @@ const Options: React.FC<OptionsProps> = (props) => {
<Select
disabled={disabled}
prefixCls={selectPrefixCls}
showSearch={false}
showSearch={showSearch}
className={`${prefixCls}-size-changer`}
optionLabelProp="children"
popupMatchSelectWidth={false}
Copy link
Member

@afc163 afc163 Aug 7, 2024

Choose a reason for hiding this comment

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

Could we pass the whole showSizeChanger object to Select? That we could customize Select as needed.

showSizeChanger={{
  disabled: true,
  className: 'xxx',
  style={{ width: 300 }}
  popupMatchSelectWidth: true,
}}

Copy link
Author

Choose a reason for hiding this comment

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

@afc163 Will address these changes next commit

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.

5 participants