Fix "ActionListItem without onClick has shadow" bug (#831)

* Fixed shadow appearance without onClick bug

* Organized ActionList test suite and added xtest for hover check

* Minor layout change to keep header and items aligned

* CHANGELOG update

Author: Elliot

CHANGELOG.md

@@ -12,6 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `Menu` closes by default on `MenuItem` click
 - Provide `@types/styled-system` as a package dependency
 
+### Fixed
+
+- `ActionListItem` no longer have shadow and cursor: pointer without an onClick
+- `ActionListItemColumn` aligns with header columns
+
 ## [0.7.28] - 2020-04-20
 
 ### Added

packages/components/src/ActionList/ActionList.test.tsx

@@ -107,6 +107,34 @@ const actionListWithNoHeader = (
   </ActionList>
 )
 
+const handleActionClick = jest.fn()
+const handleListItemClick = jest.fn()
+const clickableItems = data.map(({ id, name, type }) => {
+  const availableActions = (
+    <>
+      <ActionListItemAction onClick={handleActionClick}>
+        View Profile
+      </ActionListItemAction>
+    </>
+  )
+
+  return (
+    <ActionListItem
+      id={String(id)}
+      key={id}
+      actions={availableActions}
+      onClick={handleListItemClick}
+    >
+      <ActionListItemColumn>{id}</ActionListItemColumn>
+      <ActionListItemColumn>{name}</ActionListItemColumn>
+      <ActionListItemColumn>{type}</ActionListItemColumn>
+    </ActionListItem>
+  )
+})
+const actionListWithClickableRows = (
+  <ActionList columns={columns}>{clickableItems}</ActionList>
+)
+
 describe('ActionList', () => {
   let rafSpy: jest.SpyInstance<number, [FrameRequestCallback]>
 
@@ -118,6 +146,8 @@ describe('ActionList', () => {
 
   afterEach(() => {
     rafSpy.mockRestore()
+    handleActionClick.mockClear()
+    handleListItemClick.mockClear()
   })
 
   describe('General Layout', () => {
@@ -163,43 +193,13 @@ describe('ActionList', () => {
       expect(getByText('Game Designer')).toBeInTheDocument()
     })
 
-    test('Renders action menu on button click and handles clicks on list item and action', () => {
-      const handleActionClick = jest.fn()
-      const handleListItemClick = jest.fn()
-
-      const clickableItems = data.map(({ id, name, type }) => {
-        const availableActions = (
-          <>
-            <ActionListItemAction onClick={handleActionClick}>
-              View Profile
-            </ActionListItemAction>
-          </>
-        )
-
-        return (
-          <ActionListItem
-            id={String(id)}
-            key={id}
-            actions={availableActions}
-            onClick={handleListItemClick}
-          >
-            <ActionListItemColumn>{id}</ActionListItemColumn>
-            <ActionListItemColumn>{name}</ActionListItemColumn>
-            <ActionListItemColumn>{type}</ActionListItemColumn>
-          </ActionListItem>
-        )
-      })
-
+    test('Renders action menu on button click and handles action click', () => {
       const { getByRole, getByText, queryByText } = renderWithTheme(
-        <ActionList columns={columns}>{clickableItems}</ActionList>
+        actionListWithClickableRows
       )
 
       const listItemId = getByText('1')
 
-      expect(handleListItemClick.mock.calls.length).toBe(0)
-      fireEvent.click(listItemId)
-      expect(handleListItemClick.mock.calls.length).toBe(1)
-
       fireEvent(
         listItemId,
         new MouseEvent('mouseenter', {
@@ -221,6 +221,24 @@ describe('ActionList', () => {
 
       expect(queryByText('View Profile')).not.toBeInTheDocument()
     })
+
+    test('Handles item click', () => {
+      const { getByText } = renderWithTheme(actionListWithClickableRows)
+
+      const actionListItemColumn = getByText('1')
+
+      expect(handleListItemClick).toHaveBeenCalledTimes(0)
+      fireEvent.click(actionListItemColumn)
+      expect(handleListItemClick).toHaveBeenCalledTimes(1)
+    })
+
+    xtest('Item has pointer cursor and shadow when hovering over ActionListItem', () => {
+      /**
+       * mouseEnter events off of the fireEvent jest-dom function do not (currently)
+       * trigger hover styles (i.e. hover pseudo classes) on ActionListItems, which makes
+       * writing this particularly challenging at the moment.
+       */
+    })
   })
 
   describe('Sorting', () => {
@@ -252,7 +270,7 @@ describe('ActionList', () => {
     })
   })
 
-  describe.only('Selecting', () => {
+  describe('Selecting', () => {
     const onSelect = jest.fn()
     const actionListWithSelect = (
       <ActionList columns={columns} canSelect onSelect={onSelect}>

packages/components/src/ActionList/ActionListItem.tsx

@@ -53,14 +53,12 @@ const ActionListItemInternal: FC<ActionListItemProps> = ({
   )
 
   const handleOnSelect = () => onClickRowSelect && onSelect && onSelect(id)
-  const handleOnClick = (event: React.MouseEvent<HTMLDivElement>) => {
-    onClick && onClick(event)
-  }
+
   const handleClick = disabled
     ? undefined
     : onClickRowSelect
     ? handleOnSelect
-    : handleOnClick
+    : onClick || undefined
 
   const handleMenuClick = (event: React.MouseEvent<HTMLElement>) => {
     event.stopPropagation()

packages/components/src/ActionList/ActionListItemColumn.tsx

@@ -66,11 +66,11 @@ export const ActionListItemColumn = styled(ActionListItemColumnLayout)<
   display: ${(props) => (props.indicator ? 'flex' : undefined)};
   font-size: ${(props) => props.theme.fontSizes.xsmall};
   word-break: break-all;
+  overflow: hidden;
 
   ${ActionListItemColumnInnerLayout} {
     display: flex;
     flex-direction: column;
     justify-content: center;
-    overflow: hidden;
   }
 `