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(module:cascader): search correctly when a root node is a leaf node #2108

Merged

Conversation

zd5043039119
Copy link
Contributor

@zd5043039119 zd5043039119 commented Sep 6, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #2104

What is the new behavior?

If a root node is also a leaf node, it can also be searched and highlighted correctly.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #2108 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2108      +/-   ##
==========================================
+ Coverage   95.96%   95.97%   +<.01%     
==========================================
  Files         473      473              
  Lines       11516    11516              
  Branches     1535     1535              
==========================================
+ Hits        11051    11052       +1     
  Misses        132      132              
+ Partials      333      332       -1
Impacted Files Coverage Δ
components/cascader/nz-cascader.component.ts 96.55% <100%> (ø) ⬆️
components/tabs/nz-tabset.component.ts 96.58% <0%> (+0.85%) ⬆️

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 cca1fdd...3d4e440. Read the comment docs.

@zd5043039119 zd5043039119 force-pushed the cascader/root-leaf-node-filter branch 2 times, most recently from 0013a80 to aa1ec2f Compare September 6, 2018 09:26
Copy link
Member

@wzhudev wzhudev left a comment

Choose a reason for hiding this comment

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

Actually the reason is when we parse nodes in the first layer, we didn't consider that they could be leaf nodes. See line 1222 in nz-cascader.component.ts file.

@@ -1248,7 +1249,7 @@ export class NzCascaderComponent implements OnInit, OnDestroy, ControlValueAcces
path.pop();
};

this.oldColumnsHolder[ 0 ].forEach(node => loopParent(node));
this.oldColumnsHolder[ 0 ].forEach(node => isLeafNode(node) ? loopChild(node) : loopParent(node));
Copy link
Member

Choose a reason for hiding this comment

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

We cannot say a node is a leaf node just because it has no children. Though in search mode users cannot load data asynchronously, it would confuse users when they want to load asynchronously, and breaks API consistence in different modes.

@wzhudev
Copy link
Member

wzhudev commented Sep 7, 2018

@zd5043039119 You should add a test case to cover this situation.

@zd5043039119 zd5043039119 force-pushed the cascader/root-leaf-node-filter branch from aa1ec2f to 65c6268 Compare September 7, 2018 07:31
@zd5043039119 zd5043039119 force-pushed the cascader/root-leaf-node-filter branch from 65c6268 to 3d4e440 Compare September 7, 2018 08:08
@wzhudev
Copy link
Member

wzhudev commented Sep 7, 2018

LGTM

@vthinkxie vthinkxie merged commit 28556e4 into NG-ZORRO:master Sep 17, 2018
hsuanxyz pushed a commit to hsuanxyz/ng-zorro-antd that referenced this pull request Aug 5, 2020
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.

3 participants