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
merged 1 commit into from Sep 17, 2018

Conversation

Projects
None yet
3 participants
@zd5043039119
Contributor

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

This comment has been minimized.

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 zd5043039119:cascader/root-leaf-node-filter branch 2 times, most recently from 0013a80 to aa1ec2f Sep 6, 2018

@wendzhue

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));

This comment has been minimized.

@wendzhue

wendzhue Sep 7, 2018

Member

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.

@wendzhue

This comment has been minimized.

Member

wendzhue commented Sep 7, 2018

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

@zd5043039119 zd5043039119 force-pushed the zd5043039119:cascader/root-leaf-node-filter branch from aa1ec2f to 65c6268 Sep 7, 2018

@zd5043039119 zd5043039119 force-pushed the zd5043039119:cascader/root-leaf-node-filter branch from 65c6268 to 3d4e440 Sep 7, 2018

@wendzhue

This comment has been minimized.

Member

wendzhue commented Sep 7, 2018

LGTM

@vthinkxie vthinkxie merged commit 28556e4 into NG-ZORRO:master Sep 17, 2018

4 checks passed

codecov/patch 100% of diff hit (target 95.96%)
Details
codecov/project 95.97% (+<.01%) compared to cca1fdd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment