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

Cascader support limit filtered item count #13206

Merged
merged 7 commits into from
Nov 26, 2018
Merged

Cascader support limit filtered item count #13206

merged 7 commits into from
Nov 26, 2018

Conversation

zombieJ
Copy link
Member

@zombieJ zombieJ commented Nov 20, 2018

Add limit prop in showSearch. Default value: 50:

<Cascader
  {...props}
  showSearch={{ limit: 30 }}
/>

ref: #13097

@zombieJ zombieJ requested a review from afc163 November 20, 2018 05:24
@zombieJ zombieJ changed the base branch from master to feature November 20, 2018 05:24
@netlify
Copy link

netlify bot commented Nov 20, 2018

Deploy preview for ant-design ready!

Built with commit 7467013

https://deploy-preview-13206--ant-design.netlify.com

components/cascader/index.tsx Show resolved Hide resolved
return matchCount >= limit;
});
} else {
filtered = flattenOptions.filter((path) => filter(this.state.inputValue, path, names));
Copy link
Contributor

Choose a reason for hiding this comment

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

As limit is backed by a default value, a user can only specify

<Cascader showSearch={{ filter, limit: -1 }} />

to go into this branch.

I suggest just cap the limit to be non-negative and remove this branch.

@JLHwung
Copy link
Contributor

JLHwung commented Nov 20, 2018

I can tell from the implementation details that this change is actually a breaking change. The following code

<Cascader
  {...props}
/>

will now have at most 50 filtered item unless they modify their code and specify that

<Cascader
  {...props}
  showSearch={{ limit: Infinity }}
/>

It is a good idea to add default limit to Cascader. I hope that the limit itself is configurable in a project level, so people can opt in to the new behaviour without breaking other's code.

@@ -50,6 +50,7 @@ Fields in `showSearch`:
| Property | Description | Type | Default |
| -------- | ----------- | ---- | ------- |
| filter | The function will receive two arguments, inputValue and option, if the function returns true, the option will be included in the filtered set; Otherwise, it will be excluded. | `function(inputValue, path): boolean` | |
| limit | Set the count of filtered items | boolean | 50 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be of the type number? 🤔

@codecov
Copy link

codecov bot commented Nov 20, 2018

Codecov Report

Merging #13206 into feature will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           feature   #13206      +/-   ##
===========================================
+ Coverage    92.54%   92.59%   +0.05%     
===========================================
  Files          221      221              
  Lines         5751     5763      +12     
  Branches      1669     1632      -37     
===========================================
+ Hits          5322     5336      +14     
+ Misses         424      422       -2     
  Partials         5        5
Impacted Files Coverage Δ
components/cascader/index.tsx 97.17% <100%> (+0.2%) ⬆️
components/upload/UploadList.tsx 89.62% <0%> (+1.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95a76b0...7467013. Read the comment docs.

@zombieJ
Copy link
Member Author

zombieJ commented Nov 26, 2018

Add check for limit. User can input positive number to set limit or false to use filter without limit.

@zombieJ zombieJ merged commit b1d1b75 into feature Nov 26, 2018
@zombieJ zombieJ deleted the cascaderFilter branch November 26, 2018 03:29
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.

None yet

3 participants