Skip to content

fix(module:tree): nzCheckBoxChange never emitting #8038

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

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

CondensedMilk7
Copy link
Contributor

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?

The checkedKeysChange never emits a value (emit is never called in the component), so two-way binding is not possible.

Issue Number: N/A

What is the new behavior?

Upon checking or unchecking any node, the checkedKeysChange now emits all the checked keys present in tree nodes.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

The test cases in general do not account for EventEmitter emissions. I can try to add a test for it if it is necessary.

Also let me know if there's anything else that needs to be changed or added.

@zorro-bot
Copy link

zorro-bot bot commented Jul 29, 2023

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Jul 29, 2023

Codecov Report

Merging #8038 (8331c94) into master (fa0312a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 8331c94 differs from pull request most recent head c706e55. Consider uploading reports for the commit c706e55 to get more accurate results

@@            Coverage Diff             @@
##           master    #8038      +/-   ##
==========================================
+ Coverage   91.64%   91.66%   +0.01%     
==========================================
  Files         515      515              
  Lines       17640    17652      +12     
  Branches     2790     2791       +1     
==========================================
+ Hits        16167    16180      +13     
+ Misses       1175     1174       -1     
  Partials      298      298              
Files Changed Coverage Δ
components/core/tree/nz-tree-base.service.ts 84.34% <100.00%> (+0.57%) ⬆️
components/tree/tree.component.ts 95.02% <100.00%> (+0.05%) ⬆️

... and 1 file with indirect coverage changes

@CondensedMilk7
Copy link
Contributor Author

It is also possible to pass event.keys to the EventEmitter in question, but this one works differently. Namely, if a node has all of its children selected, it will only contain the key of that node, instead of all of its children.

My solution will emit all the checked keys. Let me know whichever better suits the design.

@CondensedMilk7
Copy link
Contributor Author

@simplejason @hsuanxyz could you check this?

This bug looks like an important oversight.

@simplejason simplejason merged commit a9dc205 into NG-ZORRO:master Nov 17, 2023
@Axel-Latour
Copy link

Hello @CondensedMilk7
This PR is causing some regression. It doesn't take into account the nzCheckStrictly, so it's always sending all the children, even if you only want the parent node.
Linked issue

@CondensedMilk7
Copy link
Contributor Author

CondensedMilk7 commented Jun 26, 2024

@Axel-Latour I think my comment above went under the radar as nobody responded to it. It might be related. It also doesn't help that unit tests do not account for such simple (and important) cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants