fix search when passing items #3070

adids1221 · 2024-05-13T08:59:03Z

Description

Picker fix search when using items instead of children.
usePickerSearch hook returns filteredItems now.

Changelog

Picker fix search when using items instead of children.

Additional info

MADS-4193

M-i-k-e-l · 2024-05-19T07:19:17Z

src/components/picker/PickerItem.tsx

@@ -44,7 +44,7 @@ const PickerItem = (props: PickerItemProps) => {

  const isItemDisabled = useMemo(() => {
    return !!(disabled || (!isSelected && context.selectionLimit && context.selectionLimit === selectedCounter));
-  }, [selectedCounter]);
+  }, [selectedCounter, disabled]);


Is this related to the main issue?
Why not add the other missing deps?

yes it's related.
If the disabled is not in the deps of the useMemo then the search return the disabled item as selectable item.

about the other missing deps, I'll add isSelected.
Not sure if I should add the context we can talk about it.

I don't see the issue you're talking about.

Go to the PickerScreen, in the first example change it from children to items:

<Picker placeholder="Favorite Language" floatingPlaceholder value={this.state.language} enableModalBlur={false} onChange={item => this.setState({language: item})} topBarProps={{title: 'Languages'}} // style={{color: Colors.red20}} showSearch searchPlaceholder={'Search a language'} searchStyle={{color: Colors.blue30, placeholderTextColor: Colors.grey50}} // onSearchChange={value => console.warn('value', value)} items={longOptions} />

Search for the first disabled item you see ABAP5 and see although it's disabled you can pick it when searching for it.

M-i-k-e-l · 2024-05-19T07:22:23Z

src/components/picker/PickerItemsList.tsx

@@ -35,7 +35,7 @@ const PickerItemsList = (props: PickerItemsListProps) => {
    testID
  } = props;
  const context = useContext(PickerContext);
-  const [wheelPickerValue, setWheelPickerValue] = useState<PickerSingleValue>(context.value ?? items?.[0].value);
+  const [wheelPickerValue, setWheelPickerValue] = useState<PickerSingleValue>(context.value ?? items?.[0]?.value);


Is this related to the main issue?
Can you provide an example?

Yes it related.
When searching and there are no items matching the search it cause an issue for me and crash with error of undefined value.

example = code example 🙏

Go to the PickerScreen, in the first example change it from children to items:

<Picker placeholder="Favorite Language" floatingPlaceholder value={this.state.language} enableModalBlur={false} onChange={item => this.setState({language: item})} topBarProps={{title: 'Languages'}} // style={{color: Colors.red20}} showSearch searchPlaceholder={'Search a language'} searchStyle={{color: Colors.blue30, placeholderTextColor: Colors.grey50}} // onSearchChange={value => console.warn('value', value)} items={longOptions} />

Enter some bla-bla phrase in the search box and see the difference.

M-i-k-e-l · 2024-05-19T07:42:01Z

src/components/picker/types.tsx

+    filteredChildren?: ReactNode | undefined,
+    filteredItems?: Pick<PickerItemProps, 'label' | 'value' | 'disabled'>[]


The user should not have to do something like:

onSearchChange={(value, _bla, changed) => { console.warn('value', value); console.warn('changed', changed); }}

in order to get the changes.

You can simply change the name and type of the second value (this should be changed in the API of course)

M-i-k-e-l · 2024-05-19T07:47:03Z

src/components/picker/helpers/usePickerSearch.tsx

+  const filteredItems = useMemo(() => {
+    if (showSearch && !_.isEmpty(searchValue)) {
+      return _.filter(items, item => {
+        const {label, value} = item;
+        const itemLabel = getItemLabelPresenter(label, value);
+        return !shouldFilterOut(searchValue, itemLabel);
+      });
+    }
+
+    return items;
+  }, [showSearch, searchValue, items]);


Can you reduce this code duplication?
IMO there should be a single function that uses both children and items

M-i-k-e-l · 2024-05-19T07:50:16Z

src/components/picker/index.tsx

@@ -224,7 +225,7 @@ const Picker = React.forwardRef((props: PickerProps, ref) => {
        useWheelPicker={useWheelPicker}
        mode={mode}
        useDialog={useDialog}
-        items={useItems ? items : undefined}
+        items={useItems ? filteredItems || items : undefined}


IMO this should be filteredItems and not filteredItems || items, just like below there's only {filteredChildren}

M-i-k-e-l

Another issue: the onSearchChange value we get is correct, but the children`items` is from the previous search (it's one search behind).
It does not look like a regression, but you should definitely fix it or add a ticket to fix it.

M-i-k-e-l · 2024-05-21T12:42:35Z

src/components/picker/PickerItemsList.tsx

@@ -35,7 +35,7 @@ const PickerItemsList = (props: PickerItemsListProps) => {
    testID
  } = props;
  const context = useContext(PickerContext);
-  const [wheelPickerValue, setWheelPickerValue] = useState<PickerSingleValue>(context.value ?? items?.[0].value);
+  const [wheelPickerValue, setWheelPickerValue] = useState<PickerSingleValue>(context.value ?? items?.[0]?.value);


example = code example 🙏

M-i-k-e-l · 2024-05-21T12:50:06Z

src/components/picker/PickerItem.tsx

@@ -44,7 +44,7 @@ const PickerItem = (props: PickerItemProps) => {

  const isItemDisabled = useMemo(() => {
    return !!(disabled || (!isSelected && context.selectionLimit && context.selectionLimit === selectedCounter));
-  }, [selectedCounter]);
+  }, [selectedCounter, disabled]);


I don't see the issue you're talking about.

M-i-k-e-l · 2024-05-21T12:55:08Z

src/components/picker/helpers/usePickerSearch.tsx

-        const {label, value, getItemLabel: childGetItemLabel} = child.props;
-        const itemLabel = getItemLabelPresenter(label, value, childGetItemLabel || getItemLabel);
+      return _.filter(items, item => {
+        const {label, value, getItemLabel: itemGetItemLabel} = item.props || item;


getItemLabel seems clear (clearer than itemGetItemLabel IMO)
Maybe you can rename getItemLabel to something else like getItemLabelProps (line 9) which will make this simpler.
Not sure you even need to send it (getItemLabelFunction) as an input to the filterItems function

M-i-k-e-l · 2024-05-21T13:00:18Z

src/components/picker/helpers/usePickerSearch.tsx


-    return children;
-  }, [showSearch, searchValue, children]);
+  const filteredItems = useMemo(() => filterItems(children || items, getItemLabel), [showSearch, searchValue, items]);


You probably need children as a dependency here

fix search when passing items

8cd3d40

adids1221 requested a review from M-i-k-e-l May 13, 2024 08:59

adids1221 assigned M-i-k-e-l May 13, 2024

M-i-k-e-l reviewed May 19, 2024

View reviewed changes

adids1221 added 2 commits May 19, 2024 16:43

refcator index and usePickerSearch

c17b72a

refcator the onsearch type

0e8db07

adids1221 requested a review from M-i-k-e-l May 19, 2024 14:08

M-i-k-e-l reviewed May 21, 2024

View reviewed changes

M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l May 21, 2024

adids1221 requested a review from M-i-k-e-l May 22, 2024 07:56

adids1221 assigned M-i-k-e-l and unassigned adids1221 May 22, 2024

fixed review notes

66ff510

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix search when passing items #3070

fix search when passing items #3070

adids1221 commented May 13, 2024

M-i-k-e-l May 19, 2024

adids1221 May 19, 2024

adids1221 May 19, 2024

M-i-k-e-l May 21, 2024

adids1221 May 22, 2024

M-i-k-e-l May 19, 2024

adids1221 May 19, 2024

M-i-k-e-l May 21, 2024

adids1221 May 22, 2024

M-i-k-e-l May 19, 2024

M-i-k-e-l May 19, 2024

M-i-k-e-l May 19, 2024

M-i-k-e-l left a comment

M-i-k-e-l May 21, 2024

M-i-k-e-l May 21, 2024

M-i-k-e-l May 21, 2024

M-i-k-e-l May 21, 2024

		filteredChildren?: ReactNode \| undefined,
		filteredItems?: Pick<PickerItemProps, 'label' \| 'value' \| 'disabled'>[]

fix search when passing items #3070

Are you sure you want to change the base?

fix search when passing items #3070

Conversation

adids1221 commented May 13, 2024

Description

Changelog

Additional info

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-i-k-e-l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment