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(comp:tree): searchKeys is affected by the side effects of other v… #1483

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

liuzaijiang
Copy link
Contributor

@liuzaijiang liuzaijiang commented Mar 3, 2023

…ariables

fix #1462

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added/updated or not needed
  • Docs and demo have been added/updated or not needed

What is the current behavior?

What is the new behavior?

此issue中的demo由于设置了动态disabled,导致修改到了mergedNodeMap.value值,而searchKeyscomputed中被mergedNodeMap.value收集到了依赖,所以更新了searchKeys,进而影响到了expandedKeys; 所以把computed改为watch,使得searchKeys仅被props.searchValue影响

Other information

{
immediate: true,
},
)

return { searchedKeys }
}
Copy link

Choose a reason for hiding this comment

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

with a brief code review

In this code patch, the main changes are importing watch from vue and replacing computed with useState from @idux/cdk/utils.
It also modifies the logic of searching nodes by keywords.

The first thing to check is if the changes made are syntactically correct. For example, does the function useSearchable still compile and run? Are there any errors in the code? If not, then the syntax is correct.

Next, we should check for any potential bugs, such as whether the search function works correctly for all conditions. Is it possible for the setSearchedKeys function to not be called when the searchValue changes, or for it to be called twice?

Finally, we should look for any potential improvements that could be made. For example, is there a way to make the code more efficient or readable? Could the logic of the search function be simplified?

@idux-bot
Copy link

idux-bot bot commented Mar 3, 2023

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #1483 (b9382af) into main (82ce25b) will not change coverage.
The diff coverage is n/a.

❗ Current head b9382af differs from pull request most recent head 0da4641. Consider uploading reports for the commit 0da4641 to get more accurate results

@@           Coverage Diff           @@
##             main    #1483   +/-   ##
=======================================
  Coverage   92.80%   92.80%           
=======================================
  Files         331      331           
  Lines       30661    30661           
  Branches     3527     3527           
=======================================
  Hits        28454    28454           
  Misses       2207     2207           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@danranVm danranVm left a comment

Choose a reason for hiding this comment

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

这样修改会导致,改了 dataSource 不生效。

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