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:cascader): searchValue not working after CascaderPanel added #1487

Merged
merged 1 commit into from Mar 6, 2023

Conversation

sallerli1
Copy link
Contributor

after adding IxCascaderPanel, searching isn't working

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?

新增 IxCascaderPanel 之后,搜索不起作用了

What is the new behavior?

修复以上问题

Other information

after adding `IxCascaderPanel`, searching isn't working
@idux-bot
Copy link

idux-bot bot commented Mar 6, 2023

This preview will be available after the AzureCI is passed.

@@ -146,7 +146,7 @@ export default defineComponent({
/>
)

const panelProps = usePanelProps(props, setOverlayOpened)
const panelProps = usePanelProps(props, inputValue, setOverlayOpened)
const handleSearchInput = (evt: Event) => {
const { value } = evt.target as HTMLInputElement
setInputValue(value)
Copy link

Choose a reason for hiding this comment

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

the code review

First we will check if there are any typo errors or syntax errors in the code. We can't see any syntax errors, but it looks like the parameter "inputValue" is missing in the line below:

const panelProps = usePanelProps(props, setOverlayOpened)

We should add the missing parameter "inputValue" to the line:

const panelProps = usePanelProps(props, inputValue, setOverlayOpened)

Next, let's check if there are any potential bugs that could arise from this code. In the line below, we can see that the value of an event target is being retrieved and set as the inputValue.

const { value } = evt.target as HTMLInputElement
setInputValue(value)

It is possible that the event target is not a valid HTMLInputElement and thus the code may throw an error when trying to retrieve the value. To prevent this, we should add a check to make sure that the event target is indeed a HTMLInputElement before retrieving its value.

@@ -43,6 +44,7 @@ export function usePanelProps(

searchable: props.searchable,
searchFn: props.searchFn,
searchValue: searchValue.value,
strategy: props.strategy,
virtual: props.virtual,

Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The code patch is well structured and easy to read.
  2. The use of useControlledProp in line 13 and 43 helps ensure that the props are properly initialized and updated.
  3. The addition of the searchValue parameter in line 14 and the addition of searchValue in line 44 helps to provide a better user experience.
  4. There are no obvious bugs or risks in the code patch.
  5. There may be room for improvement in terms of performance, such as replacing the use of ComputedRef with ReactiveRef.

@@ -54,7 +54,6 @@ export const cascaderPanelProps = {
onExpandedChange: [Function, Array] as PropType<MaybeArray<(expendedKeys: any[], data: CascaderData[]) => void>>,
onLoaded: [Function, Array] as PropType<MaybeArray<(loadedKeys: any[], data: CascaderData) => void>>,
onSelect: [Function, Array] as PropType<MaybeArray<(option: CascaderData, isSelected: boolean) => void>>,
onSearch: [Function, Array] as PropType<MaybeArray<(value: string) => void>>,

// private
_virtualScrollHeight: { type: Number, default: 256 },
Copy link

Choose a reason for hiding this comment

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

:

This code appears to be a section of a codebase related to cascaderPanelProps. This code is responsible for setting up various callbacks including onExpandedChange, onLoaded, and onSelect. The 'onSearch' callback has been removed in this patch.

From a code review perspective, it appears that the removal of the 'onSearch' callback was intentional and that no bugs have been introduced with this patch. However, depending on the desired behavior, some improvements could be made. For example, if the 'onSearch' callback is still necessary, then it would need to be re-introduced. Additionally, it is recommended to provide comments or documentation to explain why the 'onSearch' callback was removed. This will help future developers understand the codebase better.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #1487 (35cc16c) into main (e044390) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 35cc16c differs from pull request most recent head eb13080. Consider uploading reports for the commit eb13080 to get more accurate results

@@            Coverage Diff             @@
##             main    #1487      +/-   ##
==========================================
- Coverage   92.74%   92.74%   -0.01%     
==========================================
  Files         331      331              
  Lines       30742    30741       -1     
  Branches     3533     3533              
==========================================
- Hits        28511    28510       -1     
  Misses       2231     2231              
Impacted Files Coverage Δ
packages/components/cascader/src/types.ts 100.00% <ø> (ø)
packages/components/cascader/src/Cascader.tsx 84.68% <100.00%> (ø)

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

@danranVm danranVm merged commit 42b408b into IDuxFE:main Mar 6, 2023
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

2 participants