Picker - fieldType - fix and align to UX guidelines (#3175)

* Picker - fieldType - fix and align to UX guidelines

* fix label when undefined

Author: Inbal-Tish

demo/src/screens/componentScreens/PickerScreen.tsx

@@ -95,8 +95,8 @@ export default class PickerScreen extends Component {
     wheelPickerValue: 'java',
     dialogPickerValue: 'java',
     customModalValues: [],
-    filter: filters[0].value,
-    scheme: schemes[0].value,
+    filter: undefined,
+    scheme: undefined,
     contact: 0
   };
 
@@ -266,6 +266,7 @@ export default class PickerScreen extends Component {
           <Picker
             value={this.state.filter}
             onChange={value => this.setState({filter: value})}
+            label="Your Posts"
             placeholder="Filter posts"
             fieldType={Picker.fieldTypes.filter}
             marginB-s3
@@ -275,7 +276,7 @@ export default class PickerScreen extends Component {
             value={this.state.scheme}
             onChange={value => this.setState({scheme: value})}
             label="Color Scheme"
-            placeholder="Filter posts"
+            placeholder="Select scheme"
             fieldType={Picker.fieldTypes.settings}
             items={schemes}
           />

src/components/picker/__tests__/index.spec.tsx

@@ -2,14 +2,14 @@ import React, {useState} from 'react';
 import {act, render, waitFor, screen} from '@testing-library/react-native';
 import Picker from '../index';
 import {PickerDriver} from '../Picker.driver.new';
+
 const countries = [
   {label: 'Israel', value: 'IL'},
   {label: 'United States', value: 'US'},
   {label: 'Germany', value: 'DE'},
   {label: 'Italy', value: 'IT'},
   {label: 'Spain', value: 'ES '}
 ];
-
 const testID = 'picker';
 
 const TestCase = (props?: any) => {
@@ -159,6 +159,7 @@ describe('Picker', () => {
 
   describe('Dialog', () => {
     const dialogProps = {useDialog: true, customPickerProps: {migrateDialog: true}};
+    
     describe('Test value', () => {
       it('Get correct value of a single item', () => {
         const driver = getDriver({
@@ -257,6 +258,7 @@ describe('Picker', () => {
   describe('Picker field types', () => {
     describe('Test filter field type', () => {
       const placeholderText = 'Select a Filter';
+      
       it('should render a filter picker', () => {
         const driver = getDriver({fieldType: 'filter', placeholder: placeholderText});
         expect(driver.isOpen()).toBeFalsy();
@@ -265,28 +267,32 @@ describe('Picker', () => {
         expect(label.props.children).toEqual(placeholderText);
       });
     });
+    
     describe('Test settings field type', () => {
       const labelText = 'Settings';
       const placeholderText = 'Select a setting';
+      
       it('should render a settings picker with label', async () => {
         const driver = getDriver({fieldType: 'settings', label: labelText, placeholder: placeholderText});
-        expect(driver.isOpen()).toBeFalsy();
         const label = screen.getByTestId(`${testID}.settings.type.label`);
         const placeholder = screen.getByTestId(`${testID}.settings.type.placeholder`);
+
+        expect(driver.isOpen()).toBeFalsy();
         expect(label).toBeTruthy();
         expect(placeholder).toBeTruthy();
         expect(label.props.children).toEqual(labelText);
-        expect(placeholder.props.children).toEqual(labelText);
+        expect(placeholder.props.children).toEqual(placeholderText);
       });
 
       it('should render a settings picker with placeholder', async () => {
         const driver = getDriver({fieldType: 'settings', placeholder: placeholderText});
-        expect(driver.isOpen()).toBeFalsy();
         const label = screen.getByTestId(`${testID}.settings.type.label`);
         const placeholder = screen.getByTestId(`${testID}.settings.type.placeholder`);
+
+        expect(driver.isOpen()).toBeFalsy();
         expect(label).toBeTruthy();
         expect(placeholder).toBeTruthy();
-        expect(label.props.children).toEqual(undefined);
+        expect(label.props.children).toEqual(placeholderText);
         expect(placeholder.props.children).toEqual(placeholderText);
       });
     });

src/components/picker/helpers/useFieldType.tsx

@@ -1,27 +1,28 @@
 import _ from 'lodash';
 import React, {useMemo} from 'react';
-import Icon from '../../icon';
-import Text from '../../text';
+import {Typography, Colors} from '../../../style';
 import View from '../../view';
+import Text from '../../text';
+import Icon from '../../icon';
 import {PickerProps, PickerFieldTypes} from '../types';
-import {Typography} from '../../../style';
 
 type UseFieldTypeProps = Pick<
   PickerProps,
-  'fieldType' | 'preset' | 'trailingAccessory' | 'label' | 'placeholder' | 'style' | 'labelStyle' | 'testID'
+  'fieldType' | 'preset' | 'trailingAccessory' | 'value' | 'label' | 'placeholder' | 'style' | 'labelStyle' | 'testID'
 >;
 
 const dropdown = require('../assets/dropdown.png');
 
 const useFieldType = (props: UseFieldTypeProps) => {
-  const {fieldType, preset, trailingAccessory, label, placeholder, style, labelStyle, testID} = props;
+  const {fieldType, preset, trailingAccessory, value, placeholder, style, label, labelStyle, testID} = props;
 
   const propsByFieldType = useMemo(() => {
     if (fieldType === PickerFieldTypes.filter) {
       return {
         preset: preset || null,
         containerStyle: {flexDirection: 'row'},
-        labelStyle: Typography.text70,
+        label: label && `${label}: `,
+        labelStyle: {...Typography.text70, color: Colors.$textNeutral},
         trailingAccessory: trailingAccessory ?? <Icon marginL-s1 source={dropdown}/>
       };
     } else if (fieldType === PickerFieldTypes.settings) {
@@ -30,28 +31,28 @@ const useFieldType = (props: UseFieldTypeProps) => {
         label: undefined
       };
     }
-  }, [fieldType, preset, trailingAccessory]);
+  }, [fieldType, preset, trailingAccessory, label]);
 
   const pickerInnerInput = useMemo(() => {
     if (fieldType === PickerFieldTypes.filter) {
       return (
         <Text text70 numberOfLines={1} style={style} testID={`${testID}.filter.type.label`}>
-          {_.isEmpty(label) ? placeholder : label}
+          {_.isEmpty(value) ? placeholder : value}
         </Text>
       );
     } else if (fieldType === PickerFieldTypes.settings) {
       return (
         <View flexG row spread>
           <Text text70 style={labelStyle} testID={`${testID}.settings.type.label`}>
-            {label}
+            {label || placeholder}
           </Text>
           <Text text70 $textPrimary style={style} testID={`${testID}.settings.type.placeholder`}>
-            {_.isEmpty(label) ? placeholder : label}
+            {_.isEmpty(value) ? placeholder : value}
           </Text>
         </View>
       );
     }
-  }, [style, labelStyle, fieldType, placeholder, label]);
+  }, [style, labelStyle, fieldType, placeholder, value, label, testID]);
 
   return {propsByFieldType, pickerInnerInput};
 };

src/components/picker/index.tsx

@@ -1,6 +1,4 @@
 // TODO: deprecate all places where we check if _.isPlainObject
-// TODO: deprecate getItemValue prop
-// TODO: deprecate getItemLabel prop
 // TODO: Add initialValue prop
 // TODO: consider deprecating renderCustomModal prop
 import _ from 'lodash';
@@ -80,29 +78,28 @@ const Picker = React.forwardRef((props: PickerProps, ref) => {
     items: propItems,
     ...others
   } = themeProps;
-  const {preset} = others;
+  const {preset, placeholder, style, trailingAccessory, label: propsLabel} = others;
   const {renderHeader, renderInput, renderOverlay, customPickerProps} = useNewPickerProps(themeProps);
-
   const [selectedItemPosition, setSelectedItemPosition] = useState<number>(0);
   const [items, setItems] = useState<PickerItemProps[]>(propItems || extractPickerItems(themeProps));
-
+  const pickerExpandable = useRef<ExpandableOverlayMethods>(null);
+  const pickerRef = useImperativePickerHandle(ref, pickerExpandable);
+  
+  // TODO: Remove this when migration is completed, starting of v8
+  // usePickerMigrationWarnings({children, migrate, getItemLabel, getItemValue});
+  
   useEffect(() => {
     if (propItems) {
       setItems(propItems);
     }
   }, [propItems]);
 
-  const pickerExpandable = useRef<ExpandableOverlayMethods>(null);
-
-  // TODO:  Remove this when migration is completed, starting of v8
-  // usePickerMigrationWarnings({children, migrate, getItemLabel, getItemValue});
-
-  const pickerRef = useImperativePickerHandle(ref, pickerExpandable);
   const {
     filteredChildren,
     setSearchValue,
     onSearchChange: _onSearchChange
   } = usePickerSearch({showSearch, onSearchChange, getItemLabel, children});
+  
   const {multiDraftValue, onDoneSelecting, toggleItemSelection, cancelSelect} = usePickerSelection({
     migrate,
     value,
@@ -114,8 +111,6 @@ const Picker = React.forwardRef((props: PickerProps, ref) => {
     mode
   });
 
-  const {placeholder, style, trailingAccessory, label: propsLabel} = themeProps;
-
   const {label, accessibilityInfo} = usePickerLabel({
     value,
     items,
@@ -133,7 +128,8 @@ const Picker = React.forwardRef((props: PickerProps, ref) => {
     style,
     placeholder,
     labelStyle,
-    label: label || propsLabel,
+    label: propsLabel,
+    value: label,
     testID
   });