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: Pagination sizeOptions select search handled by showSizeOptionsSearch prop #565

Closed
wants to merge 0 commits into from

Conversation

Enigama
Copy link

@Enigama Enigama commented Mar 15, 2024

Add ability to control sizeOptions select to have search or not
ant-design-issue
ant-design-pr

Copy link

vercel bot commented Mar 15, 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 Mar 18, 2024 7:08am

@Enigama
Copy link
Author

Enigama commented Mar 15, 2024

@yoyo837 @afc163

@yoyo837
Copy link
Member

yoyo837 commented Mar 15, 2024

CI failed.

@Enigama
Copy link
Author

Enigama commented Mar 15, 2024

@yoyo837
I see, can you advice how to run test and coverage locally? Is it possible?
image

@yoyo837
Copy link
Member

yoyo837 commented Mar 16, 2024

You only need to run npm run test locally. You can try deleting the node_modules and lock files and reinstalling them.

@yoyo837
Copy link
Member

yoyo837 commented Mar 16, 2024

image

@Enigama
Copy link
Author

Enigama commented Mar 16, 2024

@yoyo837 what kind of node version do you use? Mine is 18.17.0.
I've tried to delete node_modules and lock file but anyway got the same test errors.

@Enigama
Copy link
Author

Enigama commented Mar 16, 2024

@yoyo837 Weird when I run test in cloned pagination it's ok, but when in forked it's broken.

Copy link

codecov bot commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.72%. Comparing base (b89cb0e) to head (cc663c8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
+ Coverage   98.70%   98.72%   +0.01%     
==========================================
  Files           3        3              
  Lines         310      314       +4     
  Branches      137      140       +3     
==========================================
+ Hits          306      310       +4     
  Misses          4        4              

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

@Enigama
Copy link
Author

Enigama commented Mar 16, 2024

@yoyo837 Should we change snap manually?

@afc163
Copy link
Member

afc163 commented Mar 16, 2024

See #554 (comment) for API

@Enigama
Copy link
Author

Enigama commented Mar 16, 2024

@afc163 @yoyo837 if I make these changes it would lead some braking changes, doesn't it?

@afc163
Copy link
Member

afc163 commented Mar 16, 2024

Just make the original property still working but deprecated.

@yoyo837
Copy link
Member

yoyo837 commented Mar 16, 2024

@yoyo837 what kind of node version do you use? Mine is 18.17.0. I've tried to delete node_modules and lock file but anyway got the same test errors.

I'm using Nodejs v18.19.1. I think this may have nothing to do with the difference in node versions between you and me.

@Enigama
Copy link
Author

Enigama commented Mar 16, 2024

@afc163 i'm using next version of this package v4.0.5 in deprecation message

@Enigama
Copy link
Author

Enigama commented Mar 17, 2024

@afc163 @yoyo837 @MadCcc can you approve, now should work well

@Enigama
Copy link
Author

Enigama commented Mar 18, 2024

@afc163 @yoyo837 @MadCcc can we merge it?

src/interface.ts Outdated
@@ -17,10 +18,19 @@ export interface PaginationLocale {
page_size?: string;
}

export interface PaginationPageSizeChanger
extends Pick<SelectProps, 'showSearch'> {
Copy link
Member

Choose a reason for hiding this comment

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

It's not a good choice to export props from SelectProps since it's decided by selectComponentClass. Origin design is coupling. We should not make this more coupling.

We can make showSizeChanger to support renderProps:

<Pagination
  showSizeChanger={({ value, onChange }) => (
    <Select value={value} onChange={onChange} showSearch />
  )}
/>

Copy link
Author

@Enigama Enigama Mar 20, 2024

Choose a reason for hiding this comment

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

@zombieJ I don't see full picture of solution, can you be more specific?
So showSizeChanger would be a bool or react element?
What is the default behavior for pagination size changer, with search or without?
What about all other props that related to size changer?

Copy link
Member

Choose a reason for hiding this comment

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

OK. Like this:

interface PaginationProps {
  // ...
  showSizeChanger?: boolean | ({ value, onChange }) => ReactNode;
}

When user want more flexible, use render props to create the selection element.

@Enigama
Copy link
Author

Enigama commented Mar 23, 2024

@yoyo837 @afc163 can you reopen, I accidentally close PR

@yoyo837
Copy link
Member

yoyo837 commented Mar 23, 2024

We can’t reopen. Feel free to open an new one.

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.

4 participants