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.

Sign up for GitHub

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

Jump to bottom

fix(comp:cascader): searchValue not working after CascaderPanel added #1487

Merged
merged 1 commit into from
Mar 6, 2023
Merged

fix(comp:cascader): searchValue not working after CascaderPanel added #1487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/components/cascader/src/Cascader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
2 changes: 2 additions & 0 deletions packages/components/cascader/src/composables/usePanelProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { useControlledProp } from '@idux/cdk/utils'

export function usePanelProps(
props: CascaderProps,
searchValue: ComputedRef<string>,
setOverlayOpened: (opened: boolean) => void,
): ComputedRef<Partial<CascaderPanelProps>> {
const [expandedKeys, setExpandedKeys] = useControlledProp(props, 'expandedKeys')
Expand Down Expand Up @@ -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.

Expand Down
1 change: 0 additions & 1 deletion packages/components/cascader/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down