fix(List): handle onClick and onFocus on items correctly

CHANGELOG.md

@@ -20,6 +20,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ### Features
 - Export `triangle-left` and `triangle-right` icons in Teams theme @codepretty ([#785](https://github.com/stardust-ui/react/pull/785))
 
+### Fixes
+- Handle `onClick` and `onFocus` on ListItems correctly @layershifter ([#779](https://github.com/stardust-ui/react/pull/779))
+
 <!--------------------------------[ v0.19.1 ]------------------------------- -->
 ## [v0.19.1](https://github.com/stardust-ui/react/tree/v0.19.1) (2019-01-29)
 [Compare changes](https://github.com/stardust-ui/react/compare/v0.19.0...v0.19.1)

src/components/List/List.tsx

@@ -12,7 +12,7 @@ import {
   commonPropTypes,
   rtlTextContainer,
 } from '../../lib'
-import ListItem from './ListItem'
+import ListItem, { ListItemProps } from './ListItem'
 import { listBehavior } from '../../lib/accessibility'
 import { Accessibility, AccessibilityActionHandlers } from '../../lib/accessibility/types'
 import { ContainerFocusHandler } from '../../lib/accessibility/FocusHandling/FocusContainer'
@@ -88,6 +88,7 @@ class List extends AutoControlledComponent<ReactProps<ListProps>, ListState> {
   }
 
   static autoControlledProps = ['selectedIndex']
+
   getInitialAutoControlledState() {
     return { selectedIndex: -1, focusedIndex: 0 }
   }
@@ -135,6 +136,31 @@ class List extends AutoControlledComponent<ReactProps<ListProps>, ListState> {
     )
   }
 
+  handleItemOverrides = (predefinedProps: ListItemProps) => {
+    const { selectable } = this.props
+
+    return {
+      onFocus: (e: React.SyntheticEvent, itemProps: ListItemProps) => {
+        _.invoke(predefinedProps, 'onFocus', e, itemProps)
+
+        if (selectable) {
+          this.setState({ focusedIndex: itemProps.index })
+        }
+      },
+      onClick: (e: React.SyntheticEvent, itemProps: ListItemProps) => {
+        _.invoke(predefinedProps, 'onClick', e, itemProps)
+
+        if (selectable) {
+          this.trySetState({ selectedIndex: itemProps.index })
+          _.invoke(this.props, 'onSelectedIndexChange', e, {
+            ...this.props,
+            ...{ selectedIndex: itemProps.index },
+          })
+        }
+      },
+    }
+  }
+
   renderComponent({ ElementType, classes, accessibility, unhandledProps }) {
     const { children } = this.props
 
@@ -152,40 +178,34 @@ class List extends AutoControlledComponent<ReactProps<ListProps>, ListState> {
   }
 
   renderItems() {
-    const { items } = this.props
+    const { items, selectable } = this.props
     const { focusedIndex, selectedIndex } = this.state
 
     this.focusHandler.syncFocusedIndex(focusedIndex)
 
     this.itemRefs = []
 
-    return _.map(items, (item, idx) => {
+    return _.map(items, (item, index) => {
       const maybeSelectableItemProps = {} as any
 
-      if (this.props.selectable) {
+      if (selectable) {
         const ref = React.createRef()
-        this.itemRefs[idx] = ref
+        this.itemRefs[index] = ref
 
         maybeSelectableItemProps.ref = ref
-        maybeSelectableItemProps.onFocus = () => this.setState({ focusedIndex: idx })
-        maybeSelectableItemProps.onClick = e => {
-          this.trySetState({ selectedIndex: idx })
-          _.invoke(this.props, 'onSelectedIndexChange', e, {
-            ...this.props,
-            ...{ selectedIndex: idx },
-          })
-        }
-        maybeSelectableItemProps.selected = idx === selectedIndex
-        maybeSelectableItemProps.tabIndex = idx === focusedIndex ? 0 : -1
+        maybeSelectableItemProps.selected = index === selectedIndex
+        maybeSelectableItemProps.tabIndex = index === focusedIndex ? 0 : -1
       }
 
       const itemProps = {
         ..._.pick(this.props, List.itemProps),
         ...maybeSelectableItemProps,
+        index,
       }
 
       return ListItem.create(item, {
         defaultProps: itemProps,
+        overrideProps: this.handleItemOverrides,
       })
     })
   }

src/components/List/ListItem.tsx

@@ -31,6 +31,7 @@ export interface ListItemProps extends UIComponentProps, ContentComponentProps<a
   important?: boolean
   media?: any
 
+  index?: number
   /** A list item can indicate that it can be selected. */
   selectable?: boolean
 
@@ -85,6 +86,7 @@ class ListItem extends UIComponent<ReactProps<ListItemProps>, ListItemState> {
     media: PropTypes.any,
 
     selectable: PropTypes.bool,
+    index: PropTypes.number,
     selected: PropTypes.bool,
 
     truncateContent: PropTypes.bool,

test/specs/components/List/List-test.tsx

@@ -14,8 +14,8 @@ describe('List', () => {
   handlesAccessibility(List, { defaultRootRole: 'list' })
   listImplementsCollectionShorthandProp('items', ListItem, { mapsValueToProp: 'content' })
 
-  const getItems = () => [
-    { key: 'irving', content: 'Irving', onClick: jest.fn() },
+  const getItems = (onClick?: Function) => [
+    { key: 'irving', content: 'Irving', onClick },
     { key: 'skyler', content: 'Skyler' },
     { key: 'dante', content: 'Dante' },
   ]
@@ -29,15 +29,15 @@ describe('List', () => {
     })
 
     it('calls onClick handler for item', () => {
-      const items = getItems()
-      const listItems = mountWithProvider(<List items={items} />).find('ListItem')
+      const onClick = jest.fn()
+      const listItems = mountWithProvider(<List items={getItems(onClick)} />).find('ListItem')
 
       listItems
         .first()
         .find('li')
         .first()
         .simulate('click')
-      expect(items[0].onClick).toHaveBeenCalled()
+      expect(onClick).toHaveBeenCalled()
     })
   })
 
@@ -72,5 +72,22 @@ describe('List', () => {
       expect(updatedItems.at(0).props().selected).toBe(false)
       expect(updatedItems.at(1).props().selected).toBe(true)
     })
+
+    it('calls onClick handler for item if `selectable`', () => {
+      const onClick = jest.fn()
+      const onSelectedIndexChange = jest.fn()
+      const listItems = mountWithProvider(
+        <List items={getItems(onClick)} onSelectedIndexChange={onSelectedIndexChange} selectable />,
+      ).find('ListItem')
+
+      listItems
+        .first()
+        .find('li')
+        .first()
+        .simulate('click')
+
+      expect(onClick).toHaveBeenCalled()
+      expect(onSelectedIndexChange).toHaveBeenCalled()
+    })
   })
 })